Hi Andrew,
On 21/06/2022 14:30, Andrew Cooper wrote:
On 21/06/2022 13:07, Julien Grall wrote:
On 21/06/2022 12:27, Andrew Cooper wrote:
Hello,
I tried to have a half hour respite from security and push forward
with XTF-on-ARM, but the result was a mess.
https://github.com/andyhhp/xtf/commit/bc86e2d271f2107da9b1c9bc55a050dbdf07c6c6
is the absolute bare minimum stub VM, which has a zImage{32,64}
header, sets up the stack, makes one CONSOLEIO_write hypercall, and
then a clean SCHEDOP_shutdown.
There are some bugs:
1) kernel_zimage32_probe() rejects relocatable binaries, but if I
skip the check it works fine.
Hmmmm... which check are you referring to?
if ( (end - start) > size )
return -EINVAL;
Although now I think about it, the problem is subtly different.
Section Headers:
[Nr] Name Type Addr Off Size ES Flg
Lk Inf Al
[ 0] NULL 00000000 000000 000000 00
0 0 0
[ 1] .text PROGBITS 40000000 010000 000094 00 AX
0 0 4
[ 2] .data PROGBITS 40001000 011000 000000 00 WA
0 0 1
[ 3] .rodata PROGBITS 40001000 011000 000012 00 A
0 0 4
[ 4] .bss NOBITS 40002000 011012 001000 00 WA
0 0 4
end is calculated as 0x3000 which includes the bss (inc stack which is
bss page aligned), while the raw binary size is 0x1012 because it stops
at the end of .rodata.
Ok. I agree this is a bug. Can you send a patch?
Furthermore, kernel_zimage64_probe() ignores the header and assumes
the binary is relocatable.
Are you referring to bit 3 "Kernel physical placement"?
No. This:
/* Currently there is no length in the header, so just use the size */
start = 0;
end = size;
Which isn't true even for the v0 header. The field named text_offset in
Xen's code is start, and res1 is end (or size for relocatable).
Hmmm... text_offset is not the start. But I agree that res1 is the
effective size and should be used instead of the binary size.
Both probe functions fail to check the endianness marker.
AFAIU the header is little endian. So it is not clear to me why we
should check the endianess marker?
Not the endieness of the header, the endianness of the image. Both
headers have a field which should ought to be checked for != LE seeing
as Xen doesn't support big endian domains yet
Aside potential bugs, big endian OS should boot on Xen (PV protocol and
hypercalls are always litte endian).
[...]
(XEN) Hardware Dom0 halted: halting machine
which is weird. The CONSOLEIO_write fails to read the passed
pointer, despite appearing to have a ip-relative load to find the
string, while the SCHEDOP_shutdown passes its parameter fine (it's a
stack relative load).
From a brief look, your code is still running with MMU off and Cache
"off" (on armv8, it is more a bypass "cache" rather than off).
This means that you ought to be a lot more careful when
reading/writing value to avoid reading any stall data.
There are no relocation/etc so everything has well defined behaviour
even when the caches are off.
The problem is you are writing to the stack and then passing a pointer
to the stack to Xen. For hypercalls, we mandate the memory to be
cacheable (see arch-arm.h). So Xen may read a different value than what
you passed.
Other observations:
* There is no documented vCPU starting state.
See
https://github.com/torvalds/linux/blob/master/Documentation/arm64/booting.rst.
What's it got to do with Xen's vCPU starting state?
Because we are following what Image defined. Anything outside is
implementation defined and not something that an OS should rely on.
Also, that's
clearly not relevant for arm32 even if the implication is "Xen only
speaks the Linux ABI".
The interface exposed to the guest depends on the binary format used. At
the moment, we are implementing zImage, Image and U-boot. If there were
another, then the vCPU will be the same as defined by the new format.
It needs to be in docs/ (or public at a stretch) and not in the heads of
the maintainers.
Patches are welcomed.
* Qemu is infinitely easier to to use (i.e. no messing with dtb/etc)
as -kernel xen -initrd test-$foo with a oneliner change to the dtb
parsing to treat ramdisk and no kernel as the dom0 kernel. Maybe a
better change would be to modify qemu to understand multiple -kernel's.
* Xen can't load ELFs.
The support was dropped in 2018 because it was bogus and not used:
https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg00242.html
Personally, I think that zImage/Image is simple enough that
re-introducing ELF is not worth it. But I would be OK to consider
patches if you feel like writing them.
There is a massive usability improvement from being able to point normal
toolchain tools at the same binary you're trying to load.
Ditto.
Cheers,
--
Julien Grall
