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

2016-09-23 Thread Boris Ostrovsky 
On 09/23/2016 10:38 AM, Ian Jackson wrote:
> Boris Ostrovsky writes ("Re: [PATCH v5 03/21] acpi: Prevent GPL-only code 
> from seeping into non-GPL binaries"):
>> On 09/23/2016 10:24 AM, Ian Jackson wrote:
>>> But really, why not make this (or most of it) a here document (ie with
>>> <<) ?  
>> Sorry, I don't follow this. What is a "here document"?
> It's a redirection from a literal, introduced with <<.

Ah. I've never heard "here document" term.

-boris

>
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_04
>
> You probably want the version that looks like this  <<'END'
> as it doesn't do interpolation on the literal text, rather than the
> version without the single quotes (which _does_ do interpolation
> of $ and `.)
>
> Ian.


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

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

2016-09-23 Thread Jan Beulich 
>>> On 23.09.16 at 16:23,  wrote:
> On 09/23/2016 05:18 AM, Jan Beulich wrote:
>>
>>> @@ -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.
> 
> What is the difference between the two?

The latter would then lack any directory components, and hence
would become subject to lookup through $PATH, which would fail
if . is not included in it (and which would possibly do the wrong
thing even if it was).

>>> --- /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 .
>>> +#
>>> +
>>> +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.
> 
> Format string is the only reason. There are a couple of instances where
> output is formatted and I felt that having both echo and printf would
> feel inconsistent.

Hmm, I think avoiding the many \n would warrant this slight
inconsistency.

>>> +# 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?
> 
> I heard that associative arrays sometimes have portability issues but
> this is a regular one. FWIW this runs on a 2009 Fedora12.
> 
> Would 'declare -a links' help?

I don't know - honestly I'm not a shell script expert. All I can say is
that I wasn't able to find the construct above e.g. in SUS v6.

Jan


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

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

2016-09-23 Thread Ian Jackson 
Boris Ostrovsky writes ("Re: [PATCH v5 03/21] acpi: Prevent GPL-only code from 
seeping into non-GPL binaries"):
> On 09/23/2016 10:24 AM, Ian Jackson wrote:
> > But really, why not make this (or most of it) a here document (ie with
> > <<) ?  
> 
> Sorry, I don't follow this. What is a "here document"?

It's a redirection from a literal, introduced with <<.

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07_04

You probably want the version that looks like this  <<'END'
as it doesn't do interpolation on the literal text, rather than the
version without the single quotes (which _does_ do interpolation
of $ and `.)

Ian.

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

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

2016-09-23 Thread Boris Ostrovsky 
On 09/23/2016 10:24 AM, Ian Jackson wrote:
> Boris Ostrovsky writes ("Re: [PATCH v5 03/21] acpi: Prevent GPL-only code 
> from seeping into non-GPL binaries"):
 +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.
>> Format string is the only reason. There are a couple of instances where
>> output is formatted and I felt that having both echo and printf would
>> feel inconsistent.
> I don't think there is anything wrong with inconsistency.
>
> But really, why not make this (or most of it) a here document (ie with
> <<) ?  

Sorry, I don't follow this. What is a "here document"?

-boris

> That would remove a lot of the quoting clutter (and would also
> avoid any bugs where shell metacharacters or printf metacharacters are
> unintentionally written unquoted).
>
> Ian.


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

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

2016-09-23 Thread Ian Jackson 
Boris Ostrovsky writes ("Re: [PATCH v5 03/21] acpi: Prevent GPL-only code from 
seeping into non-GPL binaries"):
> >> +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.
> 
> Format string is the only reason. There are a couple of instances where
> output is formatted and I felt that having both echo and printf would
> feel inconsistent.

I don't think there is anything wrong with inconsistency.

But really, why not make this (or most of it) a here document (ie with
<<) ?  That would remove a lot of the quoting clutter (and would also
avoid any bugs where shell metacharacters or printf metacharacters are
unintentionally written unquoted).

Ian.

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

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

2016-09-23 Thread Boris Ostrovsky 
On 09/23/2016 05:18 AM, Jan Beulich wrote:
>
>> @@ -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.

What is the difference between the two?

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

Format string is the only reason. There are a couple of instances where
output is formatted and I felt that having both echo and printf would
feel inconsistent.

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

I heard that associative arrays sometimes have portability issues but
this is a regular one. FWIW this runs on a 2009 Fedora12.

Would 'declare -a links' help?

>
>> +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 $()?

No reason, that's the style I usually use. I can change to $() if that's
the preferred one.

(I will resend this patch only to avoid spamming everyone with 20
already-acked patches)

-boris


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

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

2016-09-23 Thread Ian Jackson 
Boris Ostrovsky writes ("[PATCH v5 03/21] acpi: Prevent GPL-only code from 
seeping into non-GPL binaries"):
> Some code (specifically, introduced by commit 801d469ad ("[HVM] ACPI
> support patch 3 of 4: ACPI _PRT table.")) has only been licensed under
> GPLv2. We want to prevent this code from showing up in non-GPL
> binaries which might become possible after we make ACPI builder code
> available to users other than hvmloader.
> 
> There are two pieces that we need to be careful about:
> (1) A small chunk of code in dsdt.asl that implements _PIC method
> (2) A chunk of ASL generator in mk_dsdt.c that describes with PCI
> interrupt routing.
> 
> This code will now be generated by a GPL-only script which will be
> invoked only when ACPI builder's Makefile is called with GPL variable
> set.
> 
> We also strip license header from generated ASL files to prevent
> inadverent use of those files with incorrect license.
> 
> Signed-off-by: Boris Ostrovsky 

Acked-by: Ian Jackson 

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

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

2016-09-23 Thread Ian Jackson 
Jan Beulich writes ("Re: [PATCH v5 03/21] acpi: Prevent GPL-only code from 
seeping into non-GPL binaries"):
> 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.

My understanding is that the reason for this convention in our build
system is the concern that /bin/sh might be something horribly ancient
and buggy.

Ian.

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

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

2016-09-23 Thread Jan Beulich 
>>> On 22.09.16 at 21:13,  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 .
> +#
> +
> +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"
> +

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

2016-09-22 Thread Boris Ostrovsky 
Some code (specifically, introduced by commit 801d469ad ("[HVM] ACPI
support patch 3 of 4: ACPI _PRT table.")) has only been licensed under
GPLv2. We want to prevent this code from showing up in non-GPL
binaries which might become possible after we make ACPI builder code
available to users other than hvmloader.

There are two pieces that we need to be careful about:
(1) A small chunk of code in dsdt.asl that implements _PIC method
(2) A chunk of ASL generator in mk_dsdt.c that describes with PCI
interrupt routing.

This code will now be generated by a GPL-only script which will be
invoked only when ACPI builder's Makefile is called with GPL variable
set.

We also strip license header from generated ASL files to prevent
inadverent use of those files with incorrect license.

Signed-off-by: Boris Ostrovsky 
---
Changes in v5:
* Generate GPL-only ASL with mk_dsdt_gpl.sh script
* Strip license header from generated DSDT ASL

 tools/firmware/hvmloader/Makefile|   4 +
 tools/firmware/hvmloader/acpi/Makefile   |  14 ++-
 tools/firmware/hvmloader/acpi/dsdt.asl   |  14 ---
 tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh | 110 +++
 tools/firmware/hvmloader/acpi/mk_dsdt.c  |  68 +-
 5 files changed, 126 insertions(+), 84 deletions(-)
 create mode 100755 tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh

diff --git a/tools/firmware/hvmloader/Makefile 
b/tools/firmware/hvmloader/Makefile
index 9f7357f..23b0a58 100644
--- 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
+
 .PHONY: all
 all: subdirs-all
$(MAKE) hvmloader
diff --git a/tools/firmware/hvmloader/acpi/Makefile 
b/tools/firmware/hvmloader/acpi/Makefile
index f63e734..820b8a7 100644
--- 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)
 OBJS  = $(patsubst %.c,%.o,$(C_SRC))
 
 CFLAGS += $(CFLAGS_xeninclude)
@@ -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)
cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
./mk_dsdt --debug=$(debug) --dm-version qemu-xen >> $@.$(TMP_SUFFIX)
mv -f $@.$(TMP_SUFFIX) $@
 
 # NB. awk invocation is a portable alternative to 'head -n -1'
-dsdt_%cpu.asl: dsdt.asl dsdt_acpi_info.asl mk_dsdt
+dsdt_%cpu.asl: dsdt.asl dsdt_acpi_info.asl gpl/mk_dsdt_gpl.sh mk_dsdt
awk 'NR > 1 {print s} {s=$$0}' $< > $@.$(TMP_SUFFIX)
+   sed -i '1,/\*\//{/\/\*/,/\*\//d}' $@.$(TMP_SUFFIX)
+   ./gpl/mk_dsdt_gpl.sh >> $@.$(TMP_SUFFIX)
cat dsdt_acpi_info.asl >> $@.$(TMP_SUFFIX)
./mk_dsdt --debug=$(debug) --maxcpu $*  >> $@.$(TMP_SUFFIX)
mv -f $@.$(TMP_SUFFIX) $@
+endif
 
 $(filter dsdt_%.c,$(C_SRC)): %.c: iasl %.asl
iasl -vs -p $* -tc $*.asl
diff --git a/tools/firmware/hvmloader/acpi/dsdt.asl 
b/tools/firmware/hvmloader/acpi/dsdt.asl
index 4f6db79..13811cf 100644
--- a/tools/firmware/hvmloader/acpi/dsdt.asl
+++ b/tools/firmware/hvmloader/acpi/dsdt.asl
@@ -26,20 +26,6 @@ DefinitionBlock ("DSDT.aml", "DSDT", 2, "Xen", "HVM", 0)
 Name (\APCL, 0x0001)
 Name (\PUID, 0x00)
 
-/* _S3 and _S4 are in separate SSDTs */
-Name (\_S5, Package (0x04)
-{
-0x00,  /* PM1a_CNT.SLP_TYP */
-0x00,  /* PM1b_CNT.SLP_TYP */
-0x00,  /* reserved */
-0x00   /* reserved */
-})
-
-Name(PICD, 0)
-Method(_PIC, 1)
-{
-Store(Arg0, PICD) 
-}
 
 Scope (\_SB)
 {
diff --git a/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh 
b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh
new file mode 100755
index 000..4158b99
--- /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
+#