| Message ID | 1502307870-11317-3-git-send-email-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Actually Xen does not care about ARMv8 vs ARMv7. It only care about AArch32 vs AArch64. > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > --- > > - 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,
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@epam.com> >> --- >> >> - 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, >
On 09/08/2017 22:06, Volodymyr Babchuk wrote: > 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. The term AArch32 was introduced with ARMv8 and use to refer 32-bit state. ARMv7 is only 32-bit, and therefore has only AArch32 state. > How about this: "check_conditional_inst() should properly handle SMC > instruction on both architectures (ARMv7 and ARMv8) while running in > aarch32 or aarch64 mode" ? "ARMv8 allows to trap conditional SMC from AArch32 state even if the condition check failed. Modify check_conditional_inst() to handle them." > >> 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. I don't see any problem. Bits not used are usually made RES{0,1} to allow later revision using them for new features. There are also difference between ARMv8.0, ARMv8.1, ARMv8.2. But they always ensure backward compatibility on reading or a way to detect the new feature if the kernel has to set/clear bits. In the case of the ISS for SMC, the bits used are RES0, with the new meaning 0 means the SMC is unconditional or the condition passed. This is compatible with ARMv7 because conditional SMC are only trapped when the condition check passed. Cheers,
On 10.08.17 00:22, Julien Grall wrote: > > > On 09/08/2017 22:06, Volodymyr Babchuk wrote: >> 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. > > The term AArch32 was introduced with ARMv8 and use to refer 32-bit > state. ARMv7 is only 32-bit, and therefore has only AArch32 state. Hmm, maybe it is only me, but when I see term "AArch32" I automatically think about ARMv8 only, because I know that there was no such term in ARMv7. So for me "AArch32 or AArch64 state" sounds like "It is ARMv8-only thing, no ARMv7 there". Thus, I'd prefer to leave mention about ARMv7 for clarity. Maybe, just phrase it differently. >> How about this: "check_conditional_inst() should properly handle SMC >> instruction on both architectures (ARMv7 and ARMv8) while running in >> aarch32 or aarch64 mode" ? > > "ARMv8 allows to trap conditional SMC from AArch32 state even if the > condition check failed. Modify check_conditional_inst() to handle them." > >> >>> 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. > > I don't see any problem. Bits not used are usually made RES{0,1} to > allow later revision using them for new features. Okay, I'll refrain from use word "problem" :) > There are also difference between ARMv8.0, ARMv8.1, ARMv8.2. But they > always ensure backward compatibility on reading or a way to detect the > new feature if the kernel has to set/clear bits. > > In the case of the ISS for SMC, the bits used are RES0, with the new > meaning 0 means the SMC is unconditional or the condition passed. This > is compatible with ARMv7 because conditional SMC are only trapped when > the condition check passed. I'll rewrite comments in terms of backward compatibility then.
On 11/08/17 14:26, Volodymyr Babchuk wrote: > > > On 10.08.17 00:22, Julien Grall wrote: >> >> >> On 09/08/2017 22:06, Volodymyr Babchuk wrote: >>> 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. >> >> The term AArch32 was introduced with ARMv8 and use to refer 32-bit >> state. ARMv7 is only 32-bit, and therefore has only AArch32 state. > Hmm, maybe it is only me, but when I see term "AArch32" I automatically > think about ARMv8 only, because I know that there was no such term in > ARMv7. So for me "AArch32 or AArch64 state" sounds like "It is > ARMv8-only thing, no ARMv7 there". Thus, I'd prefer to leave mention > about ARMv7 for clarity. Maybe, just phrase it differently. You are right here. For me, they are the same because ARMv8 32-bit has been designed to be compatible with the former. I would tend to use ARM 32-bit and AArch32 interchangeably. > >>> How about this: "check_conditional_inst() should properly handle SMC >>> instruction on both architectures (ARMv7 and ARMv8) while running in >>> aarch32 or aarch64 mode" ? >> >> "ARMv8 allows to trap conditional SMC from AArch32 state even if the >> condition check failed. Modify check_conditional_inst() to handle them." >> >>> >>>> 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. >> >> I don't see any problem. Bits not used are usually made RES{0,1} to >> allow later revision using them for new features. > Okay, I'll refrain from use word "problem" :) There are a few differences between the both. But I would not call that a problem as they are mostly the same. > >> There are also difference between ARMv8.0, ARMv8.1, ARMv8.2. But they >> always ensure backward compatibility on reading or a way to detect the >> new feature if the kernel has to set/clear bits. >> >> In the case of the ISS for SMC, the bits used are RES0, with the new >> meaning 0 means the SMC is unconditional or the condition passed. This >> is compatible with ARMv7 because conditional SMC are only trapped when >> the condition check passed. > I'll rewrite comments in terms of backward compatibility then. Cheers,
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. + */ + 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 */
On ARMv8, one of conditional exceptions (SMC that originates from aarch32 state) have extra field in HCR.ISS encoding: 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) This is instruction specific field, so better to add new structure 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." Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- - 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(+)