[v3,1/3] xen/flask: Drop the gen-policy.py script
diff mbox series

Message ID 20191207211634.9958-2-andrew.cooper3@citrix.com
State New
Headers show
Series
  • xen: Build fixes related to Python3
Related show

Commit Message

Andrew Cooper Dec. 7, 2019, 9:16 p.m. UTC
The script is Python 2 specific, and fails with string/binary issues with
Python 3:

  Traceback (most recent call last):
    File "gen-policy.py", line 14, in <module>
      for char in sys.stdin.read():
    File "/usr/lib/python3.5/codecs.py", line 321, in decode
      (result, consumed) = self._buffer_decode(data, self.errors, final)
  UnicodeDecodeError: 'utf-8' codec can't decode byte 0x8c in position 0: invalid start byte

Fixing the script to be compatible isn't hard, but using python here is
wasteful.  Drop the script entirely, and write a short flask-policy.S instead.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: Juergen Gross <jgross@suse.com>

v2:
 * Fix tabs vs spaces issues
v3:
 * Use % rather than @ for progbits/object, for Arm32 build.
 * Spotted by https://travis-ci.org/andyhhp/xen/builds/622085138

For 4.13.  This is a blocker to our intent to by Py3-clean in this release.

Discovered entirely accidently when testing the final patch.
---
 xen/xsm/flask/Makefile       |  6 ++----
 xen/xsm/flask/flask-policy.S | 20 ++++++++++++++++++++
 xen/xsm/flask/gen-policy.py  | 23 -----------------------
 3 files changed, 22 insertions(+), 27 deletions(-)
 create mode 100644 xen/xsm/flask/flask-policy.S
 delete mode 100644 xen/xsm/flask/gen-policy.py

Comments

George Dunlap Dec. 9, 2019, 10:38 a.m. UTC | #1
On Sat, Dec 7, 2019 at 9:17 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> The script is Python 2 specific, and fails with string/binary issues with
> Python 3:
>
>   Traceback (most recent call last):
>     File "gen-policy.py", line 14, in <module>
>       for char in sys.stdin.read():
>     File "/usr/lib/python3.5/codecs.py", line 321, in decode
>       (result, consumed) = self._buffer_decode(data, self.errors, final)
>   UnicodeDecodeError: 'utf-8' codec can't decode byte 0x8c in position 0: invalid start byte
>
> Fixing the script to be compatible isn't hard, but using python here is
> wasteful.  Drop the script entirely, and write a short flask-policy.S instead.

It might be helpful for casual reviewers to have a slightly better
explanation of what the change is; namely:

- The end goal is to have a .o file exporting one variable containing
the contents of policy.bin, and another containing its size.

- gen-policy.py generates a C file which contains the bytes of
policy.bin (and its size).  This means running python, and then a c
compiler.

- The replacement use a .S file to "include" the binary directly.
This involves only running an assembler, but has the same end effect.

This looks like a good change; but I don't know assembler well enough
to give it an R-b.

 -George
Jan Beulich Dec. 9, 2019, 1:38 p.m. UTC | #2
On 07.12.2019 22:16, Andrew Cooper wrote:
> --- /dev/null
> +++ b/xen/xsm/flask/flask-policy.S
> @@ -0,0 +1,20 @@
> +        .section .init.rodata, "a", %progbits
> +
> +/* const unsigned char xsm_flask_init_policy[] __initconst */
> +        .align 4

I'm afraid .align is not universal enough to be used here - iirc
some architectures have it alias .p2align rather than (how e.g.
x86 behaves) .balign. Looks like .p2align is available in all
binutils versions we claim to be able to be built with. (I'm
not sure the one here is needed anyway, but the one below we
surely want.)

> +        .global xsm_flask_init_policy
> +xsm_flask_init_policy:
> +        .incbin "policy.bin"
> +.Lend:
> +
> +        .type xsm_flask_init_policy, %object
> +        .size xsm_flask_init_policy, . - xsm_flask_init_policy
> +
> +/* const unsigned int __initconst xsm_flask_init_policy_size */
> +        .align 4
> +        .global xsm_flask_init_policy_size
> +xsm_flask_init_policy_size:
> +        .long .Lend - xsm_flask_init_policy

Similarly .long isn't really universal (various arches override
it in gas). Aiui .dc.l is intended to be portable (despite still
carrying the 'l' in its name, and despite even this one getting
overridden by two arches). But perhaps best to ask on the
binutils list.

Jan
Andrew Cooper Dec. 9, 2019, 5:01 p.m. UTC | #3
On 09/12/2019 13:38, Jan Beulich wrote:
> On 07.12.2019 22:16, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/xen/xsm/flask/flask-policy.S
>> @@ -0,0 +1,20 @@
>> +        .section .init.rodata, "a", %progbits
>> +
>> +/* const unsigned char xsm_flask_init_policy[] __initconst */
>> +        .align 4
> I'm afraid .align is not universal enough to be used here - iirc
> some architectures have it alias .p2align rather than (how e.g.
> x86 behaves) .balign. Looks like .p2align is available in all
> binutils versions we claim to be able to be built with. (I'm
> not sure the one here is needed anyway, but the one below we
> surely want.)

I can switch to p2align, but...

>
>> +        .global xsm_flask_init_policy
>> +xsm_flask_init_policy:
>> +        .incbin "policy.bin"
>> +.Lend:
>> +
>> +        .type xsm_flask_init_policy, %object
>> +        .size xsm_flask_init_policy, . - xsm_flask_init_policy
>> +
>> +/* const unsigned int __initconst xsm_flask_init_policy_size */
>> +        .align 4
>> +        .global xsm_flask_init_policy_size
>> +xsm_flask_init_policy_size:
>> +        .long .Lend - xsm_flask_init_policy
> Similarly .long isn't really universal (various arches override
> it in gas). Aiui .dc.l is intended to be portable (despite still
> carrying the 'l' in its name, and despite even this one getting
> overridden by two arches). But perhaps best to ask on the
> binutils list.

... this is not a clear or obvious way to go, not least because it makes
a different expectation that int will never change from being 32 bits. 
At least .long will work even if it becomes longer in a future toolchain.

What is used here doesn't need to be universal - it only needs to work
for the architectures we support.

If hand writing an asm file isn't considered good enough, then the other
options are a C file with inline asm incbin, or `objdump
--rename-section`.  The latter one would require a few changes elsewhere
in the code, but only for linkage purposes.

~Andrew
Daniel De Graaf Dec. 9, 2019, 6 p.m. UTC | #4
On 12/7/19 4:16 PM, Andrew Cooper wrote:
> The script is Python 2 specific, and fails with string/binary issues with
> Python 3:
> 
>    Traceback (most recent call last):
>      File "gen-policy.py", line 14, in <module>
>        for char in sys.stdin.read():
>      File "/usr/lib/python3.5/codecs.py", line 321, in decode
>        (result, consumed) = self._buffer_decode(data, self.errors, final)
>    UnicodeDecodeError: 'utf-8' codec can't decode byte 0x8c in position 0: invalid start byte
> 
> Fixing the script to be compatible isn't hard, but using python here is
> wasteful.  Drop the script entirely, and write a short flask-policy.S instead.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

With either .align or .p2align as appropriate for more assemblers.
Jan Beulich Dec. 10, 2019, 7:58 a.m. UTC | #5
On 09.12.2019 18:01, Andrew Cooper wrote:
> On 09/12/2019 13:38, Jan Beulich wrote:
>> On 07.12.2019 22:16, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/xen/xsm/flask/flask-policy.S
>>> @@ -0,0 +1,20 @@
>>> +        .section .init.rodata, "a", %progbits
>>> +
>>> +/* const unsigned char xsm_flask_init_policy[] __initconst */
>>> +        .align 4
>> I'm afraid .align is not universal enough to be used here - iirc
>> some architectures have it alias .p2align rather than (how e.g.
>> x86 behaves) .balign. Looks like .p2align is available in all
>> binutils versions we claim to be able to be built with. (I'm
>> not sure the one here is needed anyway, but the one below we
>> surely want.)
> 
> I can switch to p2align, but...
> 
>>
>>> +        .global xsm_flask_init_policy
>>> +xsm_flask_init_policy:
>>> +        .incbin "policy.bin"
>>> +.Lend:
>>> +
>>> +        .type xsm_flask_init_policy, %object
>>> +        .size xsm_flask_init_policy, . - xsm_flask_init_policy
>>> +
>>> +/* const unsigned int __initconst xsm_flask_init_policy_size */
>>> +        .align 4
>>> +        .global xsm_flask_init_policy_size
>>> +xsm_flask_init_policy_size:
>>> +        .long .Lend - xsm_flask_init_policy
>> Similarly .long isn't really universal (various arches override
>> it in gas). Aiui .dc.l is intended to be portable (despite still
>> carrying the 'l' in its name, and despite even this one getting
>> overridden by two arches). But perhaps best to ask on the
>> binutils list.
> 
> ... this is not a clear or obvious way to go, not least because it makes
> a different expectation that int will never change from being 32 bits. 
> At least .long will work even if it becomes longer in a future toolchain.

There are a few targets where .long (and .int) appear to produce 2-byte
values (at the first glance, i.e. without checking very closely).

> What is used here doesn't need to be universal - it only needs to work
> for the architectures we support.

But it also would better not break silently for some future port. How
about an equivalent to Linux'es _ASM_PTR() (say ASM_WORD()), which each
architecture has to supply explicitly?

> If hand writing an asm file isn't considered good enough, then the other
> options are a C file with inline asm incbin, or `objdump
> --rename-section`.  The latter one would require a few changes elsewhere
> in the code, but only for linkage purposes.

I'm entirely fine with an assembler source here, it just needs a little
more polishing imo.

Jan

Patch
diff mbox series

diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index f5ffab1226..7c3f381287 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -27,7 +27,8 @@  $(FLASK_H_FILES): $(FLASK_H_DEPEND)
 $(AV_H_FILES): $(AV_H_DEPEND)
 	$(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND)
 
-obj-$(CONFIG_XSM_FLASK_POLICY) += policy.o
+obj-bin-$(CONFIG_XSM_FLASK_POLICY) += flask-policy.o
+flask-policy.o: policy.bin
 
 FLASK_BUILD_DIR := $(CURDIR)
 POLICY_SRC := $(FLASK_BUILD_DIR)/xenpolicy-$(XEN_FULLVERSION)
@@ -36,9 +37,6 @@  policy.bin: FORCE
 	$(MAKE) -f $(XEN_ROOT)/tools/flask/policy/Makefile.common -C $(XEN_ROOT)/tools/flask/policy FLASK_BUILD_DIR=$(FLASK_BUILD_DIR)
 	cmp -s $(POLICY_SRC) $@ || cp $(POLICY_SRC) $@
 
-policy.c: policy.bin gen-policy.py
-	$(PYTHON) gen-policy.py < $< > $@
-
 .PHONY: clean
 clean::
 	rm -f $(ALL_H_FILES) *.o $(DEPS_RM) policy.* $(POLICY_SRC)
diff --git a/xen/xsm/flask/flask-policy.S b/xen/xsm/flask/flask-policy.S
new file mode 100644
index 0000000000..81bfc09ec2
--- /dev/null
+++ b/xen/xsm/flask/flask-policy.S
@@ -0,0 +1,20 @@ 
+        .section .init.rodata, "a", %progbits
+
+/* const unsigned char xsm_flask_init_policy[] __initconst */
+        .align 4
+        .global xsm_flask_init_policy
+xsm_flask_init_policy:
+        .incbin "policy.bin"
+.Lend:
+
+        .type xsm_flask_init_policy, %object
+        .size xsm_flask_init_policy, . - xsm_flask_init_policy
+
+/* const unsigned int __initconst xsm_flask_init_policy_size */
+        .align 4
+        .global xsm_flask_init_policy_size
+xsm_flask_init_policy_size:
+        .long .Lend - xsm_flask_init_policy
+
+        .type xsm_flask_init_policy_size, %object
+        .size xsm_flask_init_policy_size, . - xsm_flask_init_policy_size
diff --git a/xen/xsm/flask/gen-policy.py b/xen/xsm/flask/gen-policy.py
deleted file mode 100644
index c7501e4614..0000000000
--- a/xen/xsm/flask/gen-policy.py
+++ /dev/null
@@ -1,23 +0,0 @@ 
-#!/usr/bin/env python
-import sys
-
-policy_size = 0
-
-sys.stdout.write("""
-/* This file is autogenerated by gen_policy.py */
-#include <xen/init.h>
-#include <xsm/xsm.h>
-
-const unsigned char xsm_flask_init_policy[] __initconst = {
-""")
-
-for char in sys.stdin.read():
-    sys.stdout.write(" 0x%02x," % ord(char))
-    policy_size = policy_size + 1
-    if policy_size % 13 == 0:
-        sys.stdout.write("\n")
-
-sys.stdout.write("""
-};
-const unsigned int __initconst xsm_flask_init_policy_size = %d;
-""" % policy_size)