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

[Julien Grall <julien.grall@xxxxxxx>]

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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel