Re: [Xen-devel] [PATCH v5 03/21] acpi: Prevent GPL-only code from seeping into non-GPL binaries

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.16 at 21:13, <boris.ostrovsky@xxxxxxxxxx> wrote:
> --- a/tools/firmware/hvmloader/Makefile
> +++ b/tools/firmware/hvmloader/Makefile
> @@ -65,6 +65,10 @@ ROMBIOS_ROM := $(ROMBIOS_DIR)/BIOS-bochs-latest
>  ROMS += $(ROMBIOS_ROM) $(STDVGA_ROM) $(CIRRUSVGA_ROM) $(ETHERBOOT_ROMS)
>  endif
>  
> +# Certain parts of ACPI builder are GPL-only
> +GPL = y
> +export GPL

This could (and hence imo should) be a single line.

Also I think I did say so before - please use := for simple assignments
like this one.

> --- a/tools/firmware/hvmloader/acpi/Makefile
> +++ b/tools/firmware/hvmloader/acpi/Makefile
> @@ -17,7 +17,8 @@
>  XEN_ROOT = $(CURDIR)/../../../..
>  include $(XEN_ROOT)/tools/firmware/Rules.mk
>  
> -C_SRC = build.c dsdt_anycpu.c dsdt_15cpu.c static_tables.c 
> dsdt_anycpu_qemu_xen.c
> +C_SRC-$(GPL)         = build.c dsdt_anycpu.c dsdt_15cpu.c 
> dsdt_anycpu_qemu_xen.c
> +C_SRC                = build.c static_tables.c $(C_SRC-y)

I don't think the use of tabs here is really helpful. In no case should
a blank be followed by a tab.

> @@ -36,18 +37,25 @@ ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h: %.h: %.asl iasl
>  mk_dsdt: mk_dsdt.c
>       $(HOSTCC) $(HOSTCFLAGS) $(CFLAGS_xeninclude) -o $@ mk_dsdt.c
>  
> -dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl mk_dsdt
> +ifeq ($(GPL),y)
> +dsdt_anycpu_qemu_xen.asl: dsdt.asl dsdt_acpi_info.asl gpl/mk_dsdt_gpl.sh 
> mk_dsdt
>       awk 'NR > 1 {print s} {s=$$0}' $< > $@.$(TMP_SUFFIX)
> +     # Strip license comment
> +     sed -i '1,/\*\//{/\/\*/,/\*\//d}' $@.$(TMP_SUFFIX)
> +     ./gpl/mk_dsdt_gpl.sh >> $@.$(TMP_SUFFIX)

I don't think the leading ./ is necessary here, ...

>       cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
>       ./mk_dsdt --debug=$(debug) --dm-version qemu-xen >> $@.$(TMP_SUFFIX)

... other than e.g. here.

Also I think shell scripts get better invoked via $(SHELL), to avoid
running into problems when on some file system they end up without
execute permission.

> --- /dev/null
> +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh
> @@ -0,0 +1,110 @@
> +# This program is free software; you can redistribute it and/or modify it
> +# under the terms and conditions of the GNU General Public License,
> +# version 2, as published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope it will be useful, but WITHOUT
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> +# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> +# more details.
> +#
> +# You should have received a copy of the GNU General Public License along 
> with
> +# this program; If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +printf "\n    /* Beginning of GPL-only code */\n\n"
> +
> +printf "    /* _S3 and _S4 are in separate SSDTs */\n"
> +printf "    Name (\_S5, Package (0x04) {\n"
> +printf "        0x00,  /* PM1a_CNT.SLP_TYP */\n"
> +printf "        0x00,  /* PM1b_CNT.SLP_TYP */\n"
> +printf "        0x00,  /* reserved */\n"
> +printf "        0x00   /* reserved */\n"
> +printf "    })\n"
> +
> +printf "    Name(PICD, 0)\n"
> +printf "    Method(_PIC, 1) {\n"
> +printf "        Store(Arg0, PICD)\n"
> +printf "    }\n"

Wouldn't this be better readable with "echo", avoiding all the \n
instances? Actually this seems to even apply to most if not
everything further down, as so far I didn't spot a case where
you actually pass anything other than just a format string.

> +# PCI-ISA link definitions
> +
> +# BUFA: List of ISA IRQs available for linking to PCI INTx.
> +printf "    Scope ( \_SB.PCI0 )  {\n"
> +printf "        Name ( BUFA, ResourceTemplate() { IRQ(Level, ActiveLow, 
> Shared) { 5, 10, 11 } } )\n"
> +# BUFB: IRQ descriptor for returning from link-device _CRS methods.
> +printf "        Name ( BUFB, Buffer() { 0x23, 0x00, 0x00, 0x18, 0x79, 0 } 
> )\n"
> +printf "        CreateWordField ( BUFB, 0x01, IRQV )\n"
> +
> +links=(A B C D)

Is this portable?

> +for i in `seq 0 3`;
> +do

I think you want to use only one of ; and newline as separator here.
And is there a reason to prefer backquotes over the imo better
legible $()?

> +    link=${links[$i]}
> +    printf "        Device ( LNK$link ) {\n"
> +    printf "            Name ( _HID,  EISAID(\"PNP0C0F\") )\n" #PCI 
> interrupt link
> +    printf "            Name ( _UID, $((i+1)))\n"
> +    printf "            Method ( _STA, 0) {\n"
> +    printf "                If ( And(PIR$link, 0x80) ) {\n"
> +    printf "                    Return ( 0x09 )\n"
> +    printf "                }\n"
> +    printf "                Else {\n"

Since you re-write all of this anyway, please put the Else-es on the
same lines as the closing braces.

> +    printf "                    Return ( 0x0B )\n"
> +    printf "                }\n"
> +    printf "            }\n"
> +    printf "            Method ( _PRS ) {\n"
> +    printf "                Return ( BUFA )\n"
> +    printf "            }\n"
> +    printf "            Method ( _DIS ) {\n"
> +    printf "                Or ( PIR$link, 0x80, PIR$link )\n"
> +    printf "            }\n"
> +    printf "            Method ( _CRS ) {\n"
> +    printf "                And ( PIR$link, 0x0f, Local0 )\n"
> +    printf "                ShiftLeft ( 0x1, Local0, IRQV )\n"
> +    printf "                Return ( BUFB )\n"
> +    printf "            }\n"
> +    printf "            Method ( _SRS, 1 ) {\n"
> +    printf "                CreateWordField ( ARG0, 0x01, IRQ1 )\n"
> +    printf "                FindSetRightBit ( IRQ1, Local0 )\n"
> +    printf "                Decrement ( Local0 )\n"
> +    printf "                Store ( Local0, PIR$link )\n"
> +    printf "            }\n"
> +    printf "        }\n"
> +done
> +
> +# PCI interrupt routing definitions
> +# _PRT: Method to return routing table.
> +printf "        Method ( _PRT, 0 ) {\n"
> +printf "            If ( PICD ) {\n"
> +printf "                Return ( PRTA )\n"
> +printf "            }\n"
> +printf "            Return ( PRTP )\n"
> +printf "        }\n"
> +
> +# PRTP: PIC routing table (via ISA links).
> +printf "        Name(PRTP, Package() {\n"
> +for dev in `seq 1 31`;
> +do
> +    for intx in `seq 0 3`;  # INTA-D
> +    do
> +     link_idx=$(((dev+intx)&3))

Just like in C, please have blanks around binary operators.

> +     printf "            Package(){0x%04xffff, %u, \\_SB.PCI0.LNK%c, 0},\n" 
> $dev $intx ${links[$link_idx]}

I think you should really use \\\\ here, to avoid some shell having an
extension giving meaning to \_. And please split this long line.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel