Re: [PATCH v2] xen/arm: set CPSR Z bit when creating aarch32 guests

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 23/03/2022 12:36, Andrew Cooper wrote:
> On 22/03/2022 20:28, Stefano Stabellini wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >
> > The first 32 bytes of zImage are NOPs. When CONFIG_EFI is enabled in the
> > kernel, certain versions of Linux will use an UNPREDICATABLE NOP
> > encoding, sometimes resulting in an unbootable kernel. Whether the
> > resulting kernel is bootable or not depends on the processor. See commit
> > a92882a4d270 in the Linux kernel for all the details.
> >
> > All kernel releases starting from Linux 4.9 without commit a92882a4d270
> > are affected.
> >
> > Fortunately there is a simple workaround: setting the "Z" bit in CPSR
> > make it so those invalid NOP instructions are never executed. That is
> > because the instruction is conditional (not equal). So, on QEMU at
> > least, the instruction will end up to be ignored and not generate an
> > exception. Setting the "Z" bit makes those kernel versions bootable
> > again and it is harmless in the other cases.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> A discussion relevant to this came up with XTF/ARM.
>
> There is not currently a written ABI for the start state of vCPUs, and
> there needs to be.  I know x86 is in a poor shape too, but we do at
> least have some scraps of docs littered around and a plan to write some
> proper Sphinx docs.
>
> (A separate conversation was about booting from plain ELF files.  Linux
> ARM Zimage is entirely undocumented for 32bit, and discussions with RMK
> suggest that we've got bugs
Do you mind providing more details on what would be the bugs here?

> , while 64bit has insufficient documentation
> to demonstrate that our logic is correct.)
Did you actually read 
https://github.com/torvalds/linux/blob/master/Documentation/arm64/booting.rst? 

> In particular...
>
> > ---
> > Changes in v2:
> > - improve commit message
> > - add in-code comment
> > - move PSR_Z to the beginning
> > ---
> >   xen/include/public/arch-arm.h | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> > index 94b31511dd..81cee95f14 100644
> > --- a/xen/include/public/arch-arm.h
> > +++ b/xen/include/public/arch-arm.h
> > @@ -361,6 +361,7 @@ typedef uint64_t xen_callback_t;
> >   #define PSR_DBG_MASK    (1<<9)        /* arm64: Debug Exception mask */
> >   #define PSR_IT_MASK     (0x0600fc00)  /* Thumb If-Then Mask */
> >   #define PSR_JAZELLE     (1<<24)       /* Jazelle Mode */
> > +#define PSR_Z           (1<<30)       /* Zero condition flag */
> >   
> >   /* 32 bit modes */
> >   #define PSR_MODE_USR 0x10
> > @@ -383,7 +384,12 @@ typedef uint64_t xen_callback_t;
> >   #define PSR_MODE_EL1t 0x04
> >   #define PSR_MODE_EL0t 0x00
> >   
> > -#define PSR_GUEST32_INIT  (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
> > +/*
> > + * We set PSR_Z to be able to boot Linux kernel versions with an invalid
> > + * encoding of the first 8 NOP instructions. See commit a92882a4d270 in
> > + * Linux.
> > + */
> > +#define PSR_GUEST32_INIT  
> > (PSR_Z|PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
> ... this change is either breaking the ABI, or demonstrates that these
> values must not be in a public header file to begin with.
PSR_GUEST32_INIT is only exposed to the toolstack (see the ifdef above). 
It is defined in the arch-arm.h because it makes easier to keep the 
value in sync.
This is not part of the ABI and therefore we are free to change the 
value in any way we want. Setting Z is a convenient way to handle the 
Linux issue without making it too invasive.
Cheers,

--
Julien Grall