[PATCH] Arm32: MSR to SPSR needs qualification

[PATCH] Arm32: MSR to SPSR needs qualification

[Jan Beulich]

The Arm ARM's description of MSR doesn't even allow for plain "SPSR"
here, and while gas accepts this, it takes it to mean SPSR_cf. Yet
surely all of SPSR wants updating on this path, not just the lowest and
highest 8 bits.

Fixes: dfcffb128be4 ("xen/arm32: SPSR_hyp/SPSR")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -395,7 +395,7 @@ return_to_hypervisor:
         ldr r11, [sp, #UREGS_pc]
         msr ELR_hyp, r11
         ldr r11, [sp, #UREGS_cpsr]
-        msr SPSR, r11
+        msr SPSR_cxsf, r11
 #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
         /*
          * Hardening branch predictor may require to setup a different

Re: [PATCH] Arm32: MSR to SPSR needs qualification

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Fri, 11 Jun 2021, 08:55 Jan Beulich, <jbeulich@suse.com> wrote:

> The Arm ARM's description of MSR doesn't even allow for plain "SPSR"
> here, and while gas accepts this, it takes it to mean SPSR_cf. Yet
> surely all of SPSR wants updating on this path, not just the lowest and
> highest 8 bits.
>

Can you provide a reference to the Arm Arm? This would help to navigate
through the 8000 pages.

Cheers,



> Fixes: dfcffb128be4 ("xen/arm32: SPSR_hyp/SPSR")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -395,7 +395,7 @@ return_to_hypervisor:
>          ldr r11, [sp, #UREGS_pc]
>          msr ELR_hyp, r11
>          ldr r11, [sp, #UREGS_cpsr]
> -        msr SPSR, r11
> +        msr SPSR_cxsf, r11
>  #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>          /*
>           * Hardening branch predictor may require to setup a different
>
>

[-- Attachment #2: Type: text/html, Size: 1709 bytes --]

Re: [PATCH] Arm32: MSR to SPSR needs qualification

[Jan Beulich]

On 11.06.2021 10:00, Julien Grall wrote:
> On Fri, 11 Jun 2021, 08:55 Jan Beulich, <jbeulich@suse.com> wrote:
> 
>> The Arm ARM's description of MSR doesn't even allow for plain "SPSR"
>> here, and while gas accepts this, it takes it to mean SPSR_cf. Yet
>> surely all of SPSR wants updating on this path, not just the lowest and
>> highest 8 bits.
>>
> 
> Can you provide a reference to the Arm Arm? This would help to navigate
> through the 8000 pages.

Referencing the instruction page would be enough, I thought (as
even I, not being an Arm person, have no difficulty locating it).
If it isn't, how is a canonical doc ref supposed to look like on
Arm? On x86, we avoid recording document versions, section
numbers, or even page numbers in code comments or commit messages
(which isn't to say we have none of these, but we try to avoid
new ones to appear), as these tend to change with every new
version of the doc. Therefore, to me, the offending commit's "ARM
DDI 0487D.b page G8-5993" doesn't look like something I wanted to
clone from. But if you tell me otherwise, then well - so be it.

Jan

Re: [PATCH] Arm32: MSR to SPSR needs qualification

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On Fri, 11 Jun 2021, 11:16 Jan Beulich, <jbeulich@suse.com> wrote:

> On 11.06.2021 10:00, Julien Grall wrote:
> > On Fri, 11 Jun 2021, 08:55 Jan Beulich, <jbeulich@suse.com> wrote:
> >
> >> The Arm ARM's description of MSR doesn't even allow for plain "SPSR"
> >> here, and while gas accepts this, it takes it to mean SPSR_cf. Yet
> >> surely all of SPSR wants updating on this path, not just the lowest and
> >> highest 8 bits.
> >>
> >
> > Can you provide a reference to the Arm Arm? This would help to navigate
> > through the 8000 pages.
>
> Referencing the instruction page would be enough, I thought (as
> even I, not being an Arm person, have no difficulty locating it).
> If it isn't, how is a canonical doc ref supposed to look like on
> Arm? On x86, we avoid recording document versions, section
> numbers, or even page numbers in code comments or commit messages
> (which isn't to say we have none of these, but we try to avoid
> new ones to appear), as these tend to change with every new
> version of the doc. Therefore, to me, the offending commit's "ARM
> DDI 0487D.b page G8-5993" doesn't look like something I wanted to
> clone from. But if you tell me otherwise, then well - so be it.


The Arm website provides a link for nearly every revision on the specs. As
the wording can change between version, it is useful to know which spec the
understanding is based from.

 Note that for Arm32 we should quote the Armv7 spec and not the Armv8 one
because we only follow the former (there are a few small differences).



> Jan
>
>
>

[-- Attachment #2: Type: text/html, Size: 2547 bytes --]

Re: [PATCH] Arm32: MSR to SPSR needs qualification

[Jan Beulich]

On 11.06.2021 12:41, Julien Grall wrote:
> On Fri, 11 Jun 2021, 11:16 Jan Beulich, <jbeulich@suse.com> wrote:
> 
>> On 11.06.2021 10:00, Julien Grall wrote:
>>> On Fri, 11 Jun 2021, 08:55 Jan Beulich, <jbeulich@suse.com> wrote:
>>>
>>>> The Arm ARM's description of MSR doesn't even allow for plain "SPSR"
>>>> here, and while gas accepts this, it takes it to mean SPSR_cf. Yet
>>>> surely all of SPSR wants updating on this path, not just the lowest and
>>>> highest 8 bits.
>>>>
>>>
>>> Can you provide a reference to the Arm Arm? This would help to navigate
>>> through the 8000 pages.
>>
>> Referencing the instruction page would be enough, I thought (as
>> even I, not being an Arm person, have no difficulty locating it).
>> If it isn't, how is a canonical doc ref supposed to look like on
>> Arm? On x86, we avoid recording document versions, section
>> numbers, or even page numbers in code comments or commit messages
>> (which isn't to say we have none of these, but we try to avoid
>> new ones to appear), as these tend to change with every new
>> version of the doc. Therefore, to me, the offending commit's "ARM
>> DDI 0487D.b page G8-5993" doesn't look like something I wanted to
>> clone from. But if you tell me otherwise, then well - so be it.
> 
> 
> The Arm website provides a link for nearly every revision on the specs. As
> the wording can change between version, it is useful to know which spec the
> understanding is based from.
> 
>  Note that for Arm32 we should quote the Armv7 spec and not the Armv8 one
> because we only follow the former (there are a few small differences).

Thanks for having me dig out an up-to-date Armv7 spec. I find this
puzzling in particular because you didn't care to have the earlier
commit provide a v7 doc ref. Initially I did intentionally use (a
newer version of) the doc that was pointed at there (which I also
think is better structured than the v7 one).

Jan

Re: [PATCH] Arm32: MSR to SPSR needs qualification

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Fri, 11 Jun 2021, 15:02 Jan Beulich, <jbeulich@suse.com> wrote:

> On 11.06.2021 12:41, Julien Grall wrote:
> > On Fri, 11 Jun 2021, 11:16 Jan Beulich, <jbeulich@suse.com> wrote:
> >
> >> On 11.06.2021 10:00, Julien Grall wrote:
> >>> On Fri, 11 Jun 2021, 08:55 Jan Beulich, <jbeulich@suse.com> wrote:
> >>>
> >>>> The Arm ARM's description of MSR doesn't even allow for plain "SPSR"
> >>>> here, and while gas accepts this, it takes it to mean SPSR_cf. Yet
> >>>> surely all of SPSR wants updating on this path, not just the lowest
> and
> >>>> highest 8 bits.
> >>>>
> >>>
> >>> Can you provide a reference to the Arm Arm? This would help to navigate
> >>> through the 8000 pages.
> >>
> >> Referencing the instruction page would be enough, I thought (as
> >> even I, not being an Arm person, have no difficulty locating it).
> >> If it isn't, how is a canonical doc ref supposed to look like on
> >> Arm? On x86, we avoid recording document versions, section
> >> numbers, or even page numbers in code comments or commit messages
> >> (which isn't to say we have none of these, but we try to avoid
> >> new ones to appear), as these tend to change with every new
> >> version of the doc. Therefore, to me, the offending commit's "ARM
> >> DDI 0487D.b page G8-5993" doesn't look like something I wanted to
> >> clone from. But if you tell me otherwise, then well - so be it.
> >
> >
> > The Arm website provides a link for nearly every revision on the specs.
> As
> > the wording can change between version, it is useful to know which spec
> the
> > understanding is based from.
> >
> >  Note that for Arm32 we should quote the Armv7 spec and not the Armv8 one
> > because we only follow the former (there are a few small differences).
>
> Thanks for having me dig out an up-to-date Armv7 spec. I find this
> puzzling in particular because you didn't care to have the earlier
> commit provide a v7 doc ref. Initially I did intentionally use (a
> newer version of) the doc that was pointed at there (which I also
> think is better structured than the v7 one).


Well Stefano replied past midnight UK time with the reference and committed
nearly afterwards. So I didn't really have time to object...

When I asked for the reference I didn't think I needed to mention it should
be the Armv7 as he should know we only support Armv7 for 32-bit.

I didn't bother to reply afterwards. But given there is a bug and you
quoted him, I chose to make clear that reference should be Armv7 only.

Cheers,



> Jan
>
>

[-- Attachment #2: Type: text/html, Size: 3787 bytes --]