| Message ID | 20191207211634.9958-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Build fixes related to Python3 | expand |
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
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
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
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.
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
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)
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