XTF-on-ARM: Bugs

XTF-on-ARM: Bugs

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2993 bytes --]

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.

Furthermore, kernel_zimage64_probe() ignores the header and assumes the binary is relocatable.  Both probe functions fail to check the endianness marker.

2) I'm using qemu-system-arm 4.2.1 (Debian 1:4.2-3ubuntu6.21), with some parameters cribbed from the Gitlab CI smoke test, but ctxt_switch_to() exploded with undef on:

WRITE_CP32(n->arch.joscr, JOSCR);
WRITE_CP32(n->arch.jmcr, JMCR);

I'm not sure what these are (beyond Jazelle conf register), but I commented them out and it made further progress.  I have no idea if this is a Xen bug, qemu bug, or user error, but something is clearly wrong here.

3) For test-arm64-stub, I get this:

(XEN) d0: extended region 1: 0x70000000->0x80000000
(XEN) Loading zImage from 0000000048000000 to 0000000050000000-0000000050001012
(XEN) Loading d0 DTB to 0x0000000058000000-0x0000000058001c85
...
(XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
(XEN) Freed 324kB init memory.
(XEN) *** Got CONSOLEIO_write (18 bytes)
Hello from ARM64
(XEN) *** CONSOLEIO_write done
(XEN) arch/arm/traps.c:2054:d0v0 HSR=0x000000939f0045 pc=0x00000050000098 gva=0x80002ffc gpa=0x00000080002ffc
qemu-system-aarch64: terminating on signal 2

i.e. the CONSOLEIO_write hypercall completes successfully, but a trap occurs before the SCHEDOP_shutdown completes.  The full (tiny) binaries are attached, but it seems to be faulting on:

    40000098:    b81fcc3f     str    wzr, [x1, #-4]!

which (I think) is the store of 0 to the stack for the schedop shutdown reason.

4) For test-arm32-stub under either the 32bit or 64bit Xen, I get:

(XEN) Freed 348kB init memory.
(XEN) *** Got CONSOLEIO_write (18 bytes)
(XEN) *** got fault
(XEN) *** Got SCHEDOP_shutdown, 0
(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).


Other observations:

* There is no documented vCPU starting state.
* 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.

Some of these bugs might be mine, but at a minimum 1 is a bug in Xen and needs fixing.  Any ideas?

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 3705 bytes --]

[-- Attachment #2: test-arm64-stub-syms --]
[-- Type: application/octet-stream, Size: 72520 bytes --]

[-- Attachment #3: test-arm32-stub-syms --]
[-- Type: application/octet-stream, Size: 72048 bytes --]

Re: XTF-on-ARM: Bugs

[Julien Grall]

On 21/06/2022 12:27, Andrew Cooper wrote:
> Hello,

Hi,

> 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?

> 
> Furthermore, kernel_zimage64_probe() ignores the header and assumes the binary is relocatable.

Are you referring to bit 3 "Kernel physical placement"?

> 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?

> 
> 2) I'm using qemu-system-arm 4.2.1 (Debian 1:4.2-3ubuntu6.21), with some parameters cribbed from the Gitlab CI smoke test, but ctxt_switch_to() exploded with undef on:
> 
> WRITE_CP32(n->arch.joscr, JOSCR);
> WRITE_CP32(n->arch.jmcr, JMCR);
> 
> I'm not sure what these are (beyond Jazelle conf register), but I commented them out and it made further progress.  I have no idea if this is a Xen bug, qemu bug, or user error, but something is clearly wrong here.

I suspect the QEMU version is a bit too old to support 32-bit 
virtualization. Can you try a newer one?

> 
> 3) For test-arm64-stub, I get this:
> 
> (XEN) d0: extended region 1: 0x70000000->0x80000000
> (XEN) Loading zImage from 0000000048000000 to 0000000050000000-0000000050001012
> (XEN) Loading d0 DTB to 0x0000000058000000-0x0000000058001c85
> ...
> (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
> (XEN) Freed 324kB init memory.
> (XEN) *** Got CONSOLEIO_write (18 bytes)
> Hello from ARM64
> (XEN) *** CONSOLEIO_write done
> (XEN) arch/arm/traps.c:2054:d0v0 HSR=0x000000939f0045 pc=0x00000050000098 gva=0x80002ffc gpa=0x00000080002ffc

Looking at the log above, GPA belong to neither the kernel, extended 
region or DTB.

> qemu-system-aarch64: terminating on signal 2
> 
> i.e. the CONSOLEIO_write hypercall completes successfully, but a trap occurs before the SCHEDOP_shutdown completes.  The full (tiny) binaries are attached, but it seems to be faulting on:
> 
>      40000098:    b81fcc3f     str    wzr, [x1, #-4]!
> 
> which (I think) is the store of 0 to the stack for the schedop shutdown reason.

AFAICT the stack is meant to be right next after the kernel. However, 
the fault above suggest that the value is not even close.

> 
> 4) For test-arm32-stub under either the 32bit or 64bit Xen, I get:
> 
> (XEN) Freed 348kB init memory.
> (XEN) *** Got CONSOLEIO_write (18 bytes)
> (XEN) *** got fault
> (XEN) *** Got SCHEDOP_shutdown, 0

Where are those messages coming from?

> (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.

> Other observations:
> 
> * There is no documented vCPU starting state.

See 
https://github.com/torvalds/linux/blob/master/Documentation/arm64/booting.rst.

> * 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.

Cheers,

-- 
Julien Grall

Re: XTF-on-ARM: Bugs

[Michal Orzel]

Hi Andrew,

On 21.06.2022 13: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.
> 
> Furthermore, kernel_zimage64_probe() ignores the header and assumes the binary is relocatable.  Both probe functions fail to check the endianness marker.
> 
> 2) I'm using qemu-system-arm 4.2.1 (Debian 1:4.2-3ubuntu6.21), with some parameters cribbed from the Gitlab CI smoke test, but ctxt_switch_to() exploded with undef on:
> 
> WRITE_CP32(n->arch.joscr, JOSCR);
> WRITE_CP32(n->arch.jmcr, JMCR);
> 
> I'm not sure what these are (beyond Jazelle conf register), but I commented them out and it made further progress.  I have no idea if this is a Xen bug, qemu bug, or user error, but something is clearly wrong here.
> 
> 3) For test-arm64-stub, I get this:
> 
> (XEN) d0: extended region 1: 0x70000000->0x80000000
> (XEN) Loading zImage from 0000000048000000 to 0000000050000000-0000000050001012
> (XEN) Loading d0 DTB to 0x0000000058000000-0x0000000058001c85
> ...
> (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
> (XEN) Freed 324kB init memory.
> (XEN) *** Got CONSOLEIO_write (18 bytes)
> Hello from ARM64
> (XEN) *** CONSOLEIO_write done
> (XEN) arch/arm/traps.c:2054:d0v0 HSR=0x000000939f0045 pc=0x00000050000098 gva=0x80002ffc gpa=0x00000080002ffc
> qemu-system-aarch64: terminating on signal 2
> 
> i.e. the CONSOLEIO_write hypercall completes successfully, but a trap occurs before the SCHEDOP_shutdown completes.  The full (tiny) binaries are attached, but it seems to be faulting on:
> 
>     40000098:    b81fcc3f     str    wzr, [x1, #-4]!
> 
> which (I think) is the store of 0 to the stack for the schedop shutdown reason.
> 
> 4) For test-arm32-stub under either the 32bit or 64bit Xen, I get:
> 
> (XEN) Freed 348kB init memory.
> (XEN) *** Got CONSOLEIO_write (18 bytes)
> (XEN) *** got fault
> (XEN) *** Got SCHEDOP_shutdown, 0
> (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).
> 
> 
> Other observations:
> 
> * There is no documented vCPU starting state.
> * 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.
> 
> Some of these bugs might be mine, but at a minimum 1 is a bug in Xen and needs fixing.  Any ideas?
> 
> ~Andrew

FWICT Xen does not support booting ELF images so I'm not sure why do you want to use relocatable binaries.

Apart from that I'd suggest to use my patches that are tested and work fine to prevent working on the same thing.
FWICS you are based on some old patches from v1 while the new pull request is already there since March:
https://github.com/andyhhp/xtf/pull/6

This PR contains fixes for findings reported by Julien and Christopher during v1 review.

Cheers,
Michal

Re: XTF-on-ARM: Bugs

[Andrew Cooper]

On 21/06/2022 13:07, Julien Grall wrote:
>
>
> On 21/06/2022 12:27, Andrew Cooper wrote:
>> Hello,
>
> Hi,
>
>> 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.

>
>>
>> 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).

Kernel placement only pertains to whether the image should be 2M aligned
or not.

>
>> 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.

>
>>
>> 2) I'm using qemu-system-arm 4.2.1 (Debian 1:4.2-3ubuntu6.21), with
>> some parameters cribbed from the Gitlab CI smoke test, but
>> ctxt_switch_to() exploded with undef on:
>>
>> WRITE_CP32(n->arch.joscr, JOSCR);
>> WRITE_CP32(n->arch.jmcr, JMCR);
>>
>> I'm not sure what these are (beyond Jazelle conf register), but I
>> commented them out and it made further progress.  I have no idea if
>> this is a Xen bug, qemu bug, or user error, but something is clearly
>> wrong here.
>
> I suspect the QEMU version is a bit too old to support 32-bit
> virtualization. Can you try a newer one?

Not easily right now, but (other than these two issues), it appears to
work fine.  The 32bit XTF guest is executing correctly (to a first
approximation) based on the fact that it does make the two expected
hypercalls.

At a guess, it's
https://patchwork.ozlabs.org/project/qemu-devel/patch/20191216110904.30815-25-peter.maydell@linaro.org/

>
>>
>> 3) For test-arm64-stub, I get this:
>>
>> (XEN) d0: extended region 1: 0x70000000->0x80000000
>> (XEN) Loading zImage from 0000000048000000 to
>> 0000000050000000-0000000050001012
>> (XEN) Loading d0 DTB to 0x0000000058000000-0x0000000058001c85
>> ...
>> (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch
>> input)
>> (XEN) Freed 324kB init memory.
>> (XEN) *** Got CONSOLEIO_write (18 bytes)
>> Hello from ARM64
>> (XEN) *** CONSOLEIO_write done
>> (XEN) arch/arm/traps.c:2054:d0v0 HSR=0x000000939f0045
>> pc=0x00000050000098 gva=0x80002ffc gpa=0x00000080002ffc
>
> Looking at the log above, GPA belong to neither the kernel, extended
> region or DTB.
>
>> qemu-system-aarch64: terminating on signal 2
>>
>> i.e. the CONSOLEIO_write hypercall completes successfully, but a trap
>> occurs before the SCHEDOP_shutdown completes.  The full (tiny)
>> binaries are attached, but it seems to be faulting on:
>>
>>      40000098:    b81fcc3f     str    wzr, [x1, #-4]!
>>
>> which (I think) is the store of 0 to the stack for the schedop
>> shutdown reason.
>
> AFAICT the stack is meant to be right next after the kernel. However,
> the fault above suggest that the value is not even close.
>
>>
>> 4) For test-arm32-stub under either the 32bit or 64bit Xen, I get:
>>
>> (XEN) Freed 348kB init memory.
>> (XEN) *** Got CONSOLEIO_write (18 bytes)
>> (XEN) *** got fault
>> (XEN) *** Got SCHEDOP_shutdown, 0
>
> Where are those messages coming from?

My debugging, so I can see what's going on.  The "got fault" was
copy_from_user() EFAULT path.

>
>> (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.

This is the point of the minimum viable binary.  It's to have something
trivial we can develop the build system around.

>
>> 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?  Also, that's
clearly not relevant for arm32 even if the implication is "Xen only
speaks the Linux ABI".

It needs to be in docs/ (or public at a stretch) and not in the heads of
the maintainers.

>
>> * 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.

~Andrew

Re: XTF-on-ARM: Bugs

[Julien Grall]

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

Re: XTF-on-ARM: Bugs

[Christopher Clark]

On Tue, Jun 21, 2022 at 7:05 AM Julien Grall <julien@xen.org> wrote:
>
> 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.

Hi all - I've been quiet on this front but have actually been plugging
away at this in the meantime, educating myself on the details of the
Arm init code. I have made arm32 XTF tests run under qemu, with XTF
result code reporting working via PV console and reading domid from
very basic Xenstore, so far with the MMU off but a bringup of that
part-implemented. I am following this thread and planning to be
continuing work on this later this week.

Christopher

> >>> 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

Re: XTF-on-ARM: Bugs

[Bertrand Marquis]

Hi Andrew and Christopher,

I will not dig into the details of the issues you currently have
but it seems you are trying to re-do the work we already did
and have been using for quite a while.

Currently we maintain the xtf on arm code in gitlab and we
recently rebased it on the latest xtf master:
https://gitlab.com/xen-project/people/bmarquis/xtf

If possible I would suggest to start from there.

Cheers
Bertrand

Re: XTF-on-ARM: Bugs

[Andrew Cooper]

On 22/06/2022 13:32, Bertrand Marquis wrote:
> Hi Andrew and Christopher,
>
> I will not dig into the details of the issues you currently have
> but it seems you are trying to re-do the work we already did
> and have been using for quite a while.
>
> Currently we maintain the xtf on arm code in gitlab and we
> recently rebased it on the latest xtf master:
> https://gitlab.com/xen-project/people/bmarquis/xtf
>
> If possible I would suggest to start from there.

Sorry to be blunt, but no.  I've requested several times for that series
to be broken down into something which is actually reviewable, and
because that has not been done, I'm doing it at the fastest pace my
other priorities allow.

Notice how 2/3 of the patches in the past year have been bits
specifically carved out of the ARM series, or improvements to prevent
the ARM series introducing technical debt.  Furthermore, you've not
taken the "build ARM in CI" patch that I wrote specifically for you to
be part of the series, and you've got breakages to x86 from rebasing.

At this point, I am not interested in seeing any work which is not
morphing (and mostly pruning) the arm-wip branch down into a set of
clean build system modifications that can bootstrap the
as-minimal-as-I-can-make-it stub.

As it turns out, I've found the arm64 bug (it was a typo in asm), and
the arm32 bug (issue with the compiler flags, affecting all the arm
branches thus far).

When the minimum stub is working and merged, we can then see about
working up to getting the selftest working for arm32/64.

~Andrew

Re: XTF-on-ARM: Bugs

[Bertrand Marquis]

Hi Andrew,

> On 22 Jun 2022, at 17:28, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 22/06/2022 13:32, Bertrand Marquis wrote:
>> Hi Andrew and Christopher,
>> 
>> I will not dig into the details of the issues you currently have
>> but it seems you are trying to re-do the work we already did
>> and have been using for quite a while.
>> 
>> Currently we maintain the xtf on arm code in gitlab and we
>> recently rebased it on the latest xtf master:
>> https://gitlab.com/xen-project/people/bmarquis/xtf
>> 
>> If possible I would suggest to start from there.
> 
> Sorry to be blunt, but no.  I've requested several times for that series
> to be broken down into something which is actually reviewable, and
> because that has not been done, I'm doing it at the fastest pace my
> other priorities allow.

You have not requested anything, we have been asking for a year
what we could do to help without getting any answer.

> 
> Notice how 2/3 of the patches in the past year have been bits
> specifically carved out of the ARM series, or improvements to prevent
> the ARM series introducing technical debt.  Furthermore, you've not
> taken the "build ARM in CI" patch that I wrote specifically for you to
> be part of the series, and you've got breakages to x86 from rebasing.

Which patch ? Where ? There was no communication on anything like that.

> 
> At this point, I am not interested in seeing any work which is not
> morphing (and mostly pruning) the arm-wip branch down into a set of
> clean build system modifications that can bootstrap the
> as-minimal-as-I-can-make-it stub.

You cannot expect us to poll on all the possible branches that you are creating
and simply rework what we did when you do something on some branch.

We went through what you requested using GitHub and asked you at almost all
Xen Community Call what we could do to go further without getting any answer.

You are not interested in us contributing to XTF, this is understood.

Cheers
Bertrand

Re: XTF-on-ARM: Bugs

[Christopher Clark]

On Wed, Jun 22, 2022 at 9:40 AM Bertrand Marquis
<Bertrand.Marquis@arm.com> wrote:
>
> Hi Andrew,
>
> > On 22 Jun 2022, at 17:28, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> >
> > On 22/06/2022 13:32, Bertrand Marquis wrote:
> >> Hi Andrew and Christopher,
> >>
> >> I will not dig into the details of the issues you currently have
> >> but it seems you are trying to re-do the work we already did
> >> and have been using for quite a while.

Hi Bertrand - I apologise if it seems that way, and for the pace of
this being slower than you had been expecting to see.
I don't think I have actually been re-doing it and I'm grateful that
you have made your team's work available. I am working to get what you
need integrated into the upstream.

> >> Currently we maintain the xtf on arm code in gitlab and we
> >> recently rebased it on the latest xtf master:
> >> https://gitlab.com/xen-project/people/bmarquis/xtf
> >>
> >> If possible I would suggest to start from there.

Thanks - I will add this to the sources I am working with.

> >
> > Sorry to be blunt, but no.  I've requested several times for that series
> > to be broken down into something which is actually reviewable, and
> > because that has not been done, I'm doing it at the fastest pace my
> > other priorities allow.
>
> You have not requested anything, we have been asking for a year
> what we could do to help without getting any answer.

At Andy's request I had been looking into verifying the minimal
necessary pieces to get the 32-bit Arm platform implementation to
support a minimal stub test and also the XTF infrastructure (eg.
printf, xtf return code reporting) that wasn't present in the posted
work. The aim for doing that work was to build my familiarity with it
and inform judgement involved in ensuring that the initial pieces that
are merged into XTF have a maintainable structure to support each of
the architectures (and configurations of each) that we need.
It's taken longer than I wanted and it is clear that there is urgency
to getting 64-bit Arm support integrated.

>
> >
> > Notice how 2/3 of the patches in the past year have been bits
> > specifically carved out of the ARM series, or improvements to prevent
> > the ARM series introducing technical debt.  Furthermore, you've not
> > taken the "build ARM in CI" patch that I wrote specifically for you to
> > be part of the series, and you've got breakages to x86 from rebasing.
>
> Which patch ? Where ? There was no communication on anything like that.
>
> >
> > At this point, I am not interested in seeing any work which is not
> > morphing (and mostly pruning) the arm-wip branch down into a set of
> > clean build system modifications that can bootstrap the
> > as-minimal-as-I-can-make-it stub.
>
> You cannot expect us to poll on all the possible branches that you are creating
> and simply rework what we did when you do something on some branch.
>
> We went through what you requested using GitHub and asked you at almost all
> Xen Community Call what we could do to go further without getting any answer.

I will continue to be reachable via the Community Calls. I will have a
better understanding of what steps are needed next after reviewing the
branch that you have posted.

> You are not interested in us contributing to XTF, this is understood.

No, that's really not the case; your contributions are highly valued.

There's a gap that needs to be closed here between the needs of the
contributors (ie. you guys), to have platform support working and
ability to make incremental contributions for new tests, and what the
maintainer is looking for: a structure that implements an intended
design -- that is difficult for contributors to know without having
documentation for it, which is again time-consuming to produce --
supporting the many target configurations in a coherent fashion, and
introduced in small, concise logical steps. It's compounded by the
fact that this is intricate system software where hardware
platform-specific details are critical for reviewers and contributors
to understand and implement exactly correctly.

So: I'm working on closing the current gap, aiming to make meaningful
progress in the short term and can communicate with you more clearly
as to the status of that in the coming weeks.
I also think that once the initial platform support is merged, ongoing
contributions will be both easier to produce and easier to review, to
the advantage of all.

thanks

Christopher

>
> Cheers
> Bertrand
>

Re: XTF-on-ARM: Bugs

[Bertrand Marquis]

Hi Christopher,

> On 22 Jun 2022, at 21:44, Christopher Clark <christopher.w.clark@gmail.com> wrote:
> 
> On Wed, Jun 22, 2022 at 9:40 AM Bertrand Marquis
> <Bertrand.Marquis@arm.com> wrote:
>> 
>> Hi Andrew,
>> 
>>> On 22 Jun 2022, at 17:28, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>> 
>>> On 22/06/2022 13:32, Bertrand Marquis wrote:
>>>> Hi Andrew and Christopher,
>>>> 
>>>> I will not dig into the details of the issues you currently have
>>>> but it seems you are trying to re-do the work we already did
>>>> and have been using for quite a while.
> 
> Hi Bertrand - I apologise if it seems that way, and for the pace of
> this being slower than you had been expecting to see.
> I don't think I have actually been re-doing it and I'm grateful that
> you have made your team's work available. I am working to get what you
> need integrated into the upstream.

We have not been informed of anything or requested to review any
patch you are working on.
Only information we see is a claim that there is a bug.

> 
>>>> Currently we maintain the xtf on arm code in gitlab and we
>>>> recently rebased it on the latest xtf master:
>>>> https://gitlab.com/xen-project/people/bmarquis/xtf
>>>> 
>>>> If possible I would suggest to start from there.
> 
> Thanks - I will add this to the sources I am working with.

Ok

> 
>>> 
>>> Sorry to be blunt, but no. I've requested several times for that series
>>> to be broken down into something which is actually reviewable, and
>>> because that has not been done, I'm doing it at the fastest pace my
>>> other priorities allow.
>> 
>> You have not requested anything, we have been asking for a year
>> what we could do to help without getting any answer.
> 
> At Andy's request I had been looking into verifying the minimal
> necessary pieces to get the 32-bit Arm platform implementation to
> support a minimal stub test and also the XTF infrastructure (eg.
> printf, xtf return code reporting) that wasn't present in the posted
> work. The aim for doing that work was to build my familiarity with it
> and inform judgement involved in ensuring that the initial pieces that
> are merged into XTF have a maintainable structure to support each of
> the architectures (and configurations of each) that we need.
> It's taken longer than I wanted and it is clear that there is urgency
> to getting 64-bit Arm support integrated.

So you do not want our help and our code in its current form is not acceptable.

> 
>> 
>>> 
>>> Notice how 2/3 of the patches in the past year have been bits
>>> specifically carved out of the ARM series, or improvements to prevent
>>> the ARM series introducing technical debt. Furthermore, you've not
>>> taken the "build ARM in CI" patch that I wrote specifically for you to
>>> be part of the series, and you've got breakages to x86 from rebasing.
>> 
>> Which patch ? Where ? There was no communication on anything like that.
>> 
>>> 
>>> At this point, I am not interested in seeing any work which is not
>>> morphing (and mostly pruning) the arm-wip branch down into a set of
>>> clean build system modifications that can bootstrap the
>>> as-minimal-as-I-can-make-it stub.
>> 
>> You cannot expect us to poll on all the possible branches that you are creating
>> and simply rework what we did when you do something on some branch.
>> 
>> We went through what you requested using GitHub and asked you at almost all
>> Xen Community Call what we could do to go further without getting any answer.
> 
> I will continue to be reachable via the Community Calls. I will have a
> better understanding of what steps are needed next after reviewing the
> branch that you have posted.
> 
>> You are not interested in us contributing to XTF, this is understood.
> 
> No, that's really not the case; your contributions are highly valued.
> 
> There's a gap that needs to be closed here between the needs of the
> contributors (ie. you guys), to have platform support working and
> ability to make incremental contributions for new tests, and what the
> maintainer is looking for: a structure that implements an intended
> design -- that is difficult for contributors to know without having
> documentation for it, which is again time-consuming to produce --
> supporting the many target configurations in a coherent fashion, and
> introduced in small, concise logical steps. It's compounded by the
> fact that this is intricate system software where hardware
> platform-specific details are critical for reviewers and contributors
> to understand and implement exactly correctly.
> 
> So: I'm working on closing the current gap, aiming to make meaningful
> progress in the short term and can communicate with you more clearly
> as to the status of that in the coming weeks.
> I also think that once the initial platform support is merged, ongoing
> contributions will be both easier to produce and easier to review, to
> the advantage of all.

We will wait for all this to be available and at this stage we will check
if we use it or if we keep our own fork.
XTF porting was done as a base to develop Xen tests and, as it is
 now redesigned and we have no idea how, I will not have resources
anymore to assign on work on this.

All this could have been done in the open, discussing the needs and
involving the people who have tried to make this happen in the first place.

Cheers
Bertrand

Re: XTF-on-ARM: Bugs

[George Dunlap]

[-- Attachment #1.1: Type: text/plain, Size: 3648 bytes --]



> On 22 Jun 2022, at 17:28, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 22/06/2022 13:32, Bertrand Marquis wrote:
>> Hi Andrew and Christopher,
>> 
>> I will not dig into the details of the issues you currently have
>> but it seems you are trying to re-do the work we already did
>> and have been using for quite a while.
>> 
>> Currently we maintain the xtf on arm code in gitlab and we
>> recently rebased it on the latest xtf master:
>> https://gitlab.com/xen-project/people/bmarquis/xtf
>> 
>> If possible I would suggest to start from there.
> 
> Sorry to be blunt, but no.  I've requested several times for that series
> to be broken down into something which is actually reviewable, and
> because that has not been done, I'm doing it at the fastest pace my
> other priorities allow.
> 
> Notice how 2/3 of the patches in the past year have been bits
> specifically carved out of the ARM series, or improvements to prevent
> the ARM series introducing technical debt.  Furthermore, you've not
> taken the "build ARM in CI" patch that I wrote specifically for you to
> be part of the series, and you've got breakages to x86 from rebasing.
> 
> At this point, I am not interested in seeing any work which is not
> morphing (and mostly pruning) the arm-wip branch down into a set of
> clean build system modifications that can bootstrap the
> as-minimal-as-I-can-make-it stub.

Andy,

You are not in a position to dictate to anyone else what work they will be doing; particularly if that dictation means, “Do nothing until I can get a chance to get around to it.”

Bertrand and his team have their own goals and priorities they need to accomplish, just like you do: they need to get additional XTF tests working, and they need to be able to share those with partners.  They already put off that work for over a year waiting for you to get around to the architectural re-work you had in mind.  It’s not reasonable to expect them to put off indefinitely their own needs and priorities, any more than it’s reasonable for them to expect you to drop your security work and other things you’ve been working on instead of refactoring the XTF architecture.

Bertrand knew when he made the branch that the more work done on the branch, the more effort it would take to eventually merge it upstream.  You were told that they were going to create a branch and continue working on it; and you knew that the longer you delayed the architectural re-work you had in mind, the further Bertrand’s branch would drift.  It was more important for you to work on security issues; it was more important for Bertrand to get additional functionality implemented and shared.   You and Bertrand have both made your decisions with the full knowledge of the implications of your choices; there’s no point in complaining now that the natural consequences of your choices have in fact come to pass.  And it’s hypocritical to be angry at Bertrand for having priorities higher than easy merging, when you did exactly the same thing.

Bertrands response to this thread — suggesting that you begin by testing his branch to see whether the bugs you’re looking at have already been fixed there — was reasonable, polite, and cooperative.  Yours has not been; and this kind of response isn’t likely to encourage him to be cooperative in the future.

The sooner you accept that Bertrand's branch is going to continue to develop, gain more features and bugfixes, the more effectively you’ll be able to begin reducing the diff between the two, such that things can eventually be merged.

 -George


[-- Attachment #1.2: Type: text/html, Size: 16409 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 488 bytes --]