| Message ID | 20180716172712.20294-16-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Bunch of clean-up/improvement | expand |
On Mon, 16 Jul 2018, Julien Grall wrote: > GUEST_BUG_ON may be used in other files doing guest emulation. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> Given that GUEST_BUG_ON is not actually used in any other files in this patch series, I assume you are referring to one of your future series? > --- > xen/arch/arm/traps.c | 24 ------------------------ > xen/include/asm-arm/traps.h | 24 ++++++++++++++++++++++++ > 2 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index d1bf69b245..6751e4d754 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -68,30 +68,6 @@ static inline void check_stack_alignment_constraints(void) { > #endif > } > > -/* > - * GUEST_BUG_ON is intended for checking that the guest state has not been > - * corrupted in hardware and/or that the hardware behaves as we > - * believe it should (i.e. that certain traps can only occur when the > - * guest is in a particular mode). > - * > - * The intention is to limit the damage such h/w bugs (or spec > - * misunderstandings) can do by turning them into Denial of Service > - * attacks instead of e.g. information leaks or privilege escalations. > - * > - * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state! > - * > - * Compared with regular BUG_ON it dumps the guest vcpu state instead > - * of Xen's state. > - */ > -#define guest_bug_on_failed(p) \ > -do { \ > - show_execution_state(guest_cpu_user_regs()); \ > - panic("Guest Bug: %pv: '%s', line %d, file %s\n", \ > - current, p, __LINE__, __FILE__); \ > -} while (0) > -#define GUEST_BUG_ON(p) \ > - do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0) > - > #ifdef CONFIG_ARM_32 > static int debug_stack_lines = 20; > #define stack_words_per_line 8 > diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h > index 70b52d1d16..0acf7de67d 100644 > --- a/xen/include/asm-arm/traps.h > +++ b/xen/include/asm-arm/traps.h > @@ -9,6 +9,30 @@ > # include <asm/arm64/traps.h> > #endif > > +/* > + * GUEST_BUG_ON is intended for checking that the guest state has not been > + * corrupted in hardware and/or that the hardware behaves as we > + * believe it should (i.e. that certain traps can only occur when the > + * guest is in a particular mode). > + * > + * The intention is to limit the damage such h/w bugs (or spec > + * misunderstandings) can do by turning them into Denial of Service > + * attacks instead of e.g. information leaks or privilege escalations. > + * > + * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state! > + * > + * Compared with regular BUG_ON it dumps the guest vcpu state instead > + * of Xen's state. > + */ > +#define guest_bug_on_failed(p) \ > +do { \ > + show_execution_state(guest_cpu_user_regs()); \ > + panic("Guest Bug: %pv: '%s', line %d, file %s\n", \ > + current, p, __LINE__, __FILE__); \ > +} while (0) > +#define GUEST_BUG_ON(p) \ > + do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0) > + > int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr); > > void advance_pc(struct cpu_user_regs *regs, const union hsr hsr); > -- > 2.11.0 >
Hi Stefano, On 08/14/2018 10:43 PM, Stefano Stabellini wrote: > On Mon, 16 Jul 2018, Julien Grall wrote: >> GUEST_BUG_ON may be used in other files doing guest emulation. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Given that GUEST_BUG_ON is not actually used in any other files in this > patch series, I assume you are referring to one of your future series? It is going to be used later. However, I don't really refer to any series in particular. It is just that this macros could be helpful in any emulation code to catch what we think is a invalid architectural behavior. Cheers,
On Thu, 16 Aug 2018, Julien Grall wrote: > Hi Stefano, > > On 08/14/2018 10:43 PM, Stefano Stabellini wrote: > > On Mon, 16 Jul 2018, Julien Grall wrote: > > > GUEST_BUG_ON may be used in other files doing guest emulation. > > > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > Given that GUEST_BUG_ON is not actually used in any other files in this > > patch series, I assume you are referring to one of your future series? > > It is going to be used later. However, I don't really refer to any series in > particular. It is just that this macros could be helpful in any emulation code > to catch what we think is a invalid architectural behavior. It is only that a concrete use-case helps. The idea of moving GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better to move it to guest_access.h? I am just trying to point to an existing header which is more widely used across the emulators (vpl011, vgic-v3-its.c, hvm.c, ...)
Hi Stefano, On Thu, 16 Aug 2018, 19:17 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Thu, 16 Aug 2018, Julien Grall wrote: > > Hi Stefano, > > > > On 08/14/2018 10:43 PM, Stefano Stabellini wrote: > > > On Mon, 16 Jul 2018, Julien Grall wrote: > > > > GUEST_BUG_ON may be used in other files doing guest emulation. > > > > > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > > > Given that GUEST_BUG_ON is not actually used in any other files in this > > > patch series, I assume you are referring to one of your future series? > > > > It is going to be used later. However, I don't really refer to any > series in > > particular. It is just that this macros could be helpful in any > emulation code > > to catch what we think is a invalid architectural behavior. > > It is only that a concrete use-case helps. The idea of moving > GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better > to move it to guest_access.h? I am just trying to point to an existing > header which is more widely used across the emulators (vpl011, > vgic-v3-its.c, hvm.c, ...) > Definitely not in guest_access.h or any wider in includes. If you look at the implementation and documentation, it will crash the hypervisor because the goal is to catch hardware bug or misunderstanding of the spec. This is not meant to be used in emulation. We should not crash the guest/hypervisor in bad behavior but inject an exception. So trap.h is the best places for it as hardware trap are now split accros multiples files. Cheers, <div dir="auto"><div>Hi Stefano,</div><div dir="auto"><br><div class="gmail_quote" dir="auto"><div dir="ltr">On Thu, 16 Aug 2018, 19:17 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">On Thu, 16 Aug 2018, Julien Grall wrote:<br> > Hi Stefano,<br> > <br> > On 08/14/2018 10:43 PM, Stefano Stabellini wrote:<br> > > On Mon, 16 Jul 2018, Julien Grall wrote:<br> > > > GUEST_BUG_ON may be used in other files doing guest emulation.<br> > > > <br> > > > Signed-off-by: Julien Grall <<a href="mailto:julien.grall@linaro.org" target="_blank" rel="noreferrer">julien.grall@linaro.org</a>><br> > > <br> > > Given that GUEST_BUG_ON is not actually used in any other files in this<br> > > patch series, I assume you are referring to one of your future series?<br> > <br> > It is going to be used later. However, I don't really refer to any series in<br> > particular. It is just that this macros could be helpful in any emulation code<br> > to catch what we think is a invalid architectural behavior.<br> <br> It is only that a concrete use-case helps. The idea of moving<br> GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better<br> to move it to guest_access.h? I am just trying to point to an existing<br> header which is more widely used across the emulators (vpl011,<br> vgic-v3-its.c, hvm.c, ...)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Definitely not in guest_access.h or any wider in includes. If you look at the implementation and documentation, it will crash the hypervisor because the goal is to catch hardware bug or misunderstanding of the spec. This is not meant to be used in emulation. We should not crash the guest/hypervisor in bad behavior but inject an exception.</div><div dir="auto"><br></div><div dir="auto">So trap.h is the best places for it as hardware trap are now split accros multiples files.</div><div dir="auto"><br></div><div dir="auto">Cheers,</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote></div></div></div>
On Mon, 20 Aug 2018, Julien Grall wrote: > Hi Stefano, > > On Thu, 16 Aug 2018, 19:17 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Thu, 16 Aug 2018, Julien Grall wrote: > > Hi Stefano, > > > > On 08/14/2018 10:43 PM, Stefano Stabellini wrote: > > > On Mon, 16 Jul 2018, Julien Grall wrote: > > > > GUEST_BUG_ON may be used in other files doing guest emulation. > > > > > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > > > Given that GUEST_BUG_ON is not actually used in any other files in this > > > patch series, I assume you are referring to one of your future series? > > > > It is going to be used later. However, I don't really refer to any series in > > particular. It is just that this macros could be helpful in any emulation code > > to catch what we think is a invalid architectural behavior. > > It is only that a concrete use-case helps. The idea of moving > GUEST_BUG_ON is good, but where to? For instance, wouldn't it be better > to move it to guest_access.h? I am just trying to point to an existing > header which is more widely used across the emulators (vpl011, > vgic-v3-its.c, hvm.c, ...) > > > Definitely not in guest_access.h or any wider in includes. If you look at the implementation and documentation, it will crash the > hypervisor because the goal is to catch hardware bug or misunderstanding of the spec. This is not meant to be used in emulation. > We should not crash the guest/hypervisor in bad behavior but inject an exception. > > So trap.h is the best places for it as hardware trap are now split accros multiples files. OK
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index d1bf69b245..6751e4d754 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -68,30 +68,6 @@ static inline void check_stack_alignment_constraints(void) { #endif } -/* - * GUEST_BUG_ON is intended for checking that the guest state has not been - * corrupted in hardware and/or that the hardware behaves as we - * believe it should (i.e. that certain traps can only occur when the - * guest is in a particular mode). - * - * The intention is to limit the damage such h/w bugs (or spec - * misunderstandings) can do by turning them into Denial of Service - * attacks instead of e.g. information leaks or privilege escalations. - * - * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state! - * - * Compared with regular BUG_ON it dumps the guest vcpu state instead - * of Xen's state. - */ -#define guest_bug_on_failed(p) \ -do { \ - show_execution_state(guest_cpu_user_regs()); \ - panic("Guest Bug: %pv: '%s', line %d, file %s\n", \ - current, p, __LINE__, __FILE__); \ -} while (0) -#define GUEST_BUG_ON(p) \ - do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0) - #ifdef CONFIG_ARM_32 static int debug_stack_lines = 20; #define stack_words_per_line 8 diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h index 70b52d1d16..0acf7de67d 100644 --- a/xen/include/asm-arm/traps.h +++ b/xen/include/asm-arm/traps.h @@ -9,6 +9,30 @@ # include <asm/arm64/traps.h> #endif +/* + * GUEST_BUG_ON is intended for checking that the guest state has not been + * corrupted in hardware and/or that the hardware behaves as we + * believe it should (i.e. that certain traps can only occur when the + * guest is in a particular mode). + * + * The intention is to limit the damage such h/w bugs (or spec + * misunderstandings) can do by turning them into Denial of Service + * attacks instead of e.g. information leaks or privilege escalations. + * + * GUEST_BUG_ON *MUST* *NOT* be used to check for guest controllable state! + * + * Compared with regular BUG_ON it dumps the guest vcpu state instead + * of Xen's state. + */ +#define guest_bug_on_failed(p) \ +do { \ + show_execution_state(guest_cpu_user_regs()); \ + panic("Guest Bug: %pv: '%s', line %d, file %s\n", \ + current, p, __LINE__, __FILE__); \ +} while (0) +#define GUEST_BUG_ON(p) \ + do { if ( unlikely(p) ) guest_bug_on_failed(#p); } while (0) + int check_conditional_instr(struct cpu_user_regs *regs, const union hsr hsr); void advance_pc(struct cpu_user_regs *regs, const union hsr hsr);
GUEST_BUG_ON may be used in other files doing guest emulation. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/traps.c | 24 ------------------------ xen/include/asm-arm/traps.h | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 24 deletions(-)