Re: [Xen-devel] [PATCH v2 2/4] arm: processor: add new struct hsr_smc32 into hsr union

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi Julien,

On 09.08.17 23:34, Julien Grall wrote:
> On 09/08/2017 20:44, Volodymyr Babchuk wrote:
> > On ARMv8, one of conditional exceptions (SMC that originates
> > from aarch32 state) have extra field in HCR.ISS encoding:
> s/aarch32/AArch32/
> s/have/has/
>
> And the register is called HSR and not HCR.
>
> > CCKNOWNPASS, bit [19]
> > Indicates whether the instruction might have failed its condition
> > code check.
> >    0 - The instruction was unconditional, or was conditional and
> >    passed  its condition code check.
> >    1 - The instruction was conditional, and might have failed its
> >    condition code check.
> > (ARM DDI 0487A.k page D7-1949)
> Please use the latest ARM ARM.
>
> > This is instruction specific field, so better to add new structure
> This is an instruction...
>
> > to union hsr. This structure describes ISS encoding for an exception
> > from SMC instruction execution in AArch32 state. But we define this
> > struct for both ARMv7 and ARMv8. The reason is described in comment
> > to the structure:
> >
> > "Nevertheless, we define this encoding for both ARMv7 and ARMv8,
> > because check_conditional_inst() should properly handle SMC
> > instruction in all modes: ARMv7, aarch32 and aarch64."
> Hmmm. There are only two existing modes: AArch32 and AArch64. ARMv7 is 
> just a version of the specification which happen to only support AArch32.
Yeah, I wondered how to formulate that better. Problem is that ARMv7
specification does not use term "AArch32". So I decided to mention ARMv7 
explicitly.
How about this: "check_conditional_inst() should properly handle SMC 
instruction on both architectures (ARMv7 and ARMv8) while running in 
aarch32 or aarch64 mode" ?
> Actually Xen does not care about ARMv8 vs ARMv7. It only care about 
> AArch32 vs AArch64.
Yes. And probably it can be problem in the future. Because, as we can 
see, there are differences between ARMv7 and ARMv8.
> > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> > ---
> >
> > - Created new stucture for HSR_EC_SMC32 instead of extending
> >   fields in hsr.cond.
> > - Added references to ARM manual.
> > - Wrote comment with rationale.
> >
> > ---
> >  xen/include/asm-arm/processor.h | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/xen/include/asm-arm/processor.h 
> > b/xen/include/asm-arm/processor.h
> > index f640d54..af4a0f7 100644
> > --- a/xen/include/asm-arm/processor.h
> > +++ b/xen/include/asm-arm/processor.h
> > @@ -488,6 +488,25 @@ union hsr {
> >          unsigned long ec:6;     /* Exception Class */
> >      } cp; /* HSR_EC_CP */
> >
> > +    /*
> > +     * This encoding is valid only for ARMv8 (ARM DDI 0487A.k pages 
> > D7-1949 and
> > +     * G6-4405). On ARMv7, encoding ISS for EC=0x13 is defined as 
> > UNK/SBZP
> > +     * (ARM DDI 0406C.c page B3-1431). UNK/SBZP means that hardware 
> > implements
> > +     * this field as Read-As-Zero.
> > +     *
> > +     * Nevertheless, we define this encoding for both ARMv7 and 
> > ARMv8, because
> > +     * check_conditional_inst() should properly handle SMC 
> > instruction in all
> > +     * modes: ARMv7, aarch32 and aarch64.
> See my comment above.
>
> > +     */
> > +    struct hsr_smc32 {
> > +        unsigned long res0:19;  /* Reserved */
> > +        unsigned long ccknownpass:1; /* Instruction passed 
> > conditional check */
> > +        unsigned long cc:4;    /* Condition Code */
> > +        unsigned long ccvalid:1;/* CC Valid */
> > +        unsigned long len:1;   /* Instruction length */
> > +        unsigned long ec:6;    /* Exception Class */
> > +    } smc32; /* HSR_EC_SMC32 */
> > +
> >  #ifdef CONFIG_ARM_64
> >      struct hsr_sysreg {
> >          unsigned long read:1;   /* Direction */
> Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel