Re: [Xen-devel] [PATCH v3 1/3] xen/flask: Drop the gen-policy.py script
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/3] xen/flask: Drop the gen-policy.py script
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 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 Acked-by: Daniel De Graaf With either .align or .p2align as appropriate for more assemblers. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/3] xen/flask: Drop the gen-policy.py script
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/3] xen/flask: Drop the gen-policy.py script
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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 1/3] xen/flask: Drop the gen-policy.py script
On Sat, Dec 7, 2019 at 9:17 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 > 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel