| Message ID | 1474658060-6449-1-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.09.16 at 21:14, <boris.ostrovsky@oracle.com> wrote: > Changes in v6: > * Replaced script's printf in most case with "here document" (for multi-line > text) or echo for single line. Left printf for formatted output. > (Note that in one case paramter expansion is necessary and so > delimiter word is intentionally not quoted). > * Replaced bash arrays with ${string:index:size} syntax. Without having looked at the patch in full yet - is this any more portable than the previous approach? I can't see any mention of it in SUSv6 / SUSv7 either. Jan
On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote: > 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 0000000..28a0dd7 > --- /dev/null > +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh > @@ -0,0 +1,116 @@ #!/bin/sh ? > +# 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/>. > +# > + > +cat <<'EndOfASL' > + /* Beginning of GPL-only code */ > + > + /* _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) > + } > +EndOfASL > + > +# PCI-ISA link definitions > +# BUFA: List of ISA IRQs available for linking to PCI INTx. > +# BUFB: IRQ descriptor for returning from link-device _CRS methods. > +cat <<'EndOfASL' > + Scope ( \_SB.PCI0 ) { > + Name ( BUFA, ResourceTemplate() { IRQ(Level, ActiveLow, Shared) { 5, 10, 11 } } ) > + Name ( BUFB, Buffer() { 0x23, 0x00, 0x00, 0x18, 0x79, 0 } ) > + CreateWordField ( BUFB, 0x01, IRQV ) > +EndOfASL > + > +links="ABCD" > + > +for i in $(seq 0 3) > +do > + link=${links:$i:1} This is not portable. Use following instead: links="A B C D" set $links for link in $@ do ... done See http://pubs.opengroup.org/onlinepubs/009696799/utilities/set.html Wei.
Wei Liu writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"):
> On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote:
> > +links="ABCD"
> > +
> > +for i in $(seq 0 3)
> > +do
> > + link=${links:$i:1}
>
> This is not portable.
>
> Use following instead:
>
> links="A B C D"
>
> set $links
> for link in $@
What's wrong with
links="A B C D"
for link in $links; do
....
?
(To my surprise I discover that there are only marginal dependencies
on bash already in xen.git, so we should probably avoid adding a new
dependency on bash.)
Ian.
On Mon, Sep 26, 2016 at 11:20:50AM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"): > > On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote: > > > +links="ABCD" > > > + > > > +for i in $(seq 0 3) > > > +do > > > + link=${links:$i:1} > > > > This is not portable. > > > > Use following instead: > > > > links="A B C D" > > > > set $links > > for link in $@ > > What's wrong with > > links="A B C D" > > for link in $links; do > .... > > ? Yes, I think this works, too. I mistakenly thought that was bash-ism. > > (To my surprise I discover that there are only marginal dependencies > on bash already in xen.git, so we should probably avoid adding a new > dependency on bash.) > Agreed. Wei. > Ian.
On 09/26/2016 02:46 AM, Jan Beulich wrote: >>>> On 23.09.16 at 21:14, <boris.ostrovsky@oracle.com> wrote: >> Changes in v6: >> * Replaced script's printf in most case with "here document" (for multi-line >> text) or echo for single line. Left printf for formatted output. >> (Note that in one case paramter expansion is necessary and so >> delimiter word is intentionally not quoted). >> * Replaced bash arrays with ${string:index:size} syntax. > Without having looked at the patch in full yet - is this any more > portable than the previous approach? I can't see any mention of > it in SUSv6 / SUSv7 either. I can't say for sure but I remember seeing this construct long time ago. Of course, this being bash, there are at least 3 ways of doing the same thing so I can also do link=`echo "A B C D" | cut -d" " -f $i` Will SUSv6 understand this? I don't have access to anything older than fedora 13. -boris
On 09/26/2016 06:24 AM, Wei Liu wrote: > On Mon, Sep 26, 2016 at 11:20:50AM +0100, Ian Jackson wrote: >> Wei Liu writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"): >>> On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote: >>>> +links="ABCD" >>>> + >>>> +for i in $(seq 0 3) >>>> +do >>>> + link=${links:$i:1} >>> This is not portable. >>> >>> Use following instead: >>> >>> links="A B C D" >>> >>> set $links >>> for link in $@ >> What's wrong with >> >> links="A B C D" >> >> for link in $links; do >> .... >> >> ? There are two interdependent variables that I need to print. The C equivalent is for ( i = 0; i < 4; i++ ) printf("%d %c\n", i, 'A'+i); The character value is derived from 'i', which in this example is an index into 'links' array. I suggested in response to Jan link=`echo "A B C D" | cut -d" " -f $i` -boris
On Mon, Sep 26, 2016 at 08:58:00AM -0400, Boris Ostrovsky wrote: > On 09/26/2016 06:24 AM, Wei Liu wrote: > > On Mon, Sep 26, 2016 at 11:20:50AM +0100, Ian Jackson wrote: > >> Wei Liu writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"): > >>> On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote: > >>>> +links="ABCD" > >>>> + > >>>> +for i in $(seq 0 3) > >>>> +do > >>>> + link=${links:$i:1} > >>> This is not portable. > >>> > >>> Use following instead: > >>> > >>> links="A B C D" > >>> > >>> set $links > >>> for link in $@ > >> What's wrong with > >> > >> links="A B C D" > >> > >> for link in $links; do > >> .... > >> > >> ? > > There are two interdependent variables that I need to print. The C > equivalent is > > for ( i = 0; i < 4; i++ ) > printf("%d %c\n", i, 'A'+i); > > The character value is derived from 'i', which in this example is an > index into 'links' array. > > I suggested in response to Jan > > link=`echo "A B C D" | cut -d" " -f $i` > Oh, two variables. Then you could also do: link=`echo $i | tr "1234" "ABCD"` (cut and tr are both from coreutils, so I think we're fine with using either of them) Anyway, enough of discussion about shell script. Pick the method you like. :-) Wei. > > > -boris >
>>> On 26.09.16 at 14:49, <boris.ostrovsky@oracle.com> wrote: > On 09/26/2016 02:46 AM, Jan Beulich wrote: >>>>> On 23.09.16 at 21:14, <boris.ostrovsky@oracle.com> wrote: >>> Changes in v6: >>> * Replaced script's printf in most case with "here document" (for multi-line >>> text) or echo for single line. Left printf for formatted output. >>> (Note that in one case paramter expansion is necessary and so >>> delimiter word is intentionally not quoted). >>> * Replaced bash arrays with ${string:index:size} syntax. >> Without having looked at the patch in full yet - is this any more >> portable than the previous approach? I can't see any mention of >> it in SUSv6 / SUSv7 either. > > I can't say for sure but I remember seeing this construct long time ago. > > Of course, this being bash, there are at least 3 ways of doing the same > thing so I can also do > > link=`echo "A B C D" | cut -d" " -f $i` > > Will SUSv6 understand this? Yes, it looks like it will. But you could have checked yourself. Jan
On 09/26/2016 09:08 AM, Jan Beulich wrote: >>>> On 26.09.16 at 14:49, <boris.ostrovsky@oracle.com> wrote: >> On 09/26/2016 02:46 AM, Jan Beulich wrote: >>>>>> On 23.09.16 at 21:14, <boris.ostrovsky@oracle.com> wrote: >>>> Changes in v6: >>>> * Replaced script's printf in most case with "here document" (for multi-line >>>> text) or echo for single line. Left printf for formatted output. >>>> (Note that in one case paramter expansion is necessary and so >>>> delimiter word is intentionally not quoted). >>>> * Replaced bash arrays with ${string:index:size} syntax. >>> Without having looked at the patch in full yet - is this any more >>> portable than the previous approach? I can't see any mention of >>> it in SUSv6 / SUSv7 either. >> I can't say for sure but I remember seeing this construct long time ago. >> >> Of course, this being bash, there are at least 3 ways of doing the same >> thing so I can also do >> >> link=`echo "A B C D" | cut -d" " -f $i` >> >> Will SUSv6 understand this? > Yes, it looks like it will. But you could have checked yourself. Yes, I could. But somehow I thought you were referring to a SUSE product instead of the UNIX spec. Anyway, I will hold off re-sending the patch with this fixed until you review the one I sent. -boris
>>> On 26.09.16 at 15:43, <boris.ostrovsky@oracle.com> wrote: > On 09/26/2016 09:08 AM, Jan Beulich wrote: >>>>> On 26.09.16 at 14:49, <boris.ostrovsky@oracle.com> wrote: >>> On 09/26/2016 02:46 AM, Jan Beulich wrote: >>>>>>> On 23.09.16 at 21:14, <boris.ostrovsky@oracle.com> wrote: >>>>> Changes in v6: >>>>> * Replaced script's printf in most case with "here document" (for multi-line >>>>> text) or echo for single line. Left printf for formatted output. >>>>> (Note that in one case paramter expansion is necessary and so >>>>> delimiter word is intentionally not quoted). >>>>> * Replaced bash arrays with ${string:index:size} syntax. >>>> Without having looked at the patch in full yet - is this any more >>>> portable than the previous approach? I can't see any mention of >>>> it in SUSv6 / SUSv7 either. >>> I can't say for sure but I remember seeing this construct long time ago. >>> >>> Of course, this being bash, there are at least 3 ways of doing the same >>> thing so I can also do >>> >>> link=`echo "A B C D" | cut -d" " -f $i` >>> >>> Will SUSv6 understand this? >> Yes, it looks like it will. But you could have checked yourself. > > > Yes, I could. But somehow I thought you were referring to a SUSE product > instead of the UNIX spec. > > Anyway, I will hold off re-sending the patch with this fixed until you > review the one I sent. Oh, I've looked over it already, and it looks reasonable once the comments already given by others have got addressed. Jan
On 09/26/2016 06:04 AM, Wei Liu wrote: > On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote: >> 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 0000000..28a0dd7 >> --- /dev/null >> +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh >> @@ -0,0 +1,116 @@ > #!/bin/sh ? (Just noticed this comment) I don't think this is needed since it's invoked by the Makefile as $(SHELL) gpl/mk_dsdt_gpl.sh >> $@.$(TMP_SUFFIX) -boris
On Mon, Sep 26, 2016 at 10:01:37AM -0400, Boris Ostrovsky wrote: > On 09/26/2016 06:04 AM, Wei Liu wrote: > > On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote: > >> 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 0000000..28a0dd7 > >> --- /dev/null > >> +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh > >> @@ -0,0 +1,116 @@ > > #!/bin/sh ? > > (Just noticed this comment) > > I don't think this is needed since it's invoked by the Makefile as > > $(SHELL) gpl/mk_dsdt_gpl.sh >> $@.$(TMP_SUFFIX) > Fair enough. I don't think this is large enough an issue to block this patch. Please resend this patch and provide a branch on top of staging. Wei. > > -boris > >
Boris Ostrovsky writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"):
> There are two interdependent variables that I need to print. The C
> equivalent is
>
> for ( i = 0; i < 4; i++ )
> printf("%d %c\n", i, 'A'+i);
>
> The character value is derived from 'i', which in this example is an
> index into 'links' array.
>
> I suggested in response to Jan
>
> link=`echo "A B C D" | cut -d" " -f $i`
If the indices are necessarily successive integers:
links="A B C D"
index=0
for link in $links; do
index=$(( $index + 1 ))
something with $link and $index
If the indices are arbitrary:
links="1:A 4:B 7:C 10:D"
for linkinfo in $links; do
link=${linkinfo#*:}
index=${linkinfo%%:*}
something with $link and $index
Ian.
Boris Ostrovsky writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"):
> On 09/26/2016 06:04 AM, Wei Liu wrote:
> > On Fri, Sep 23, 2016 at 03:14:20PM -0400, Boris Ostrovsky wrote:
> >> 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 0000000..28a0dd7
> >> --- /dev/null
> >> +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh
> >> @@ -0,0 +1,116 @@
> > #!/bin/sh ?
>
> (Just noticed this comment)
>
> I don't think this is needed since it's invoked by the Makefile as
>
> $(SHELL) gpl/mk_dsdt_gpl.sh >> $@.$(TMP_SUFFIX)
The script should be runnable from the command line without saying
"sh" or soemthing. So it should have a #! line and be executable.
Ian.
On 09/26/2016 10:45 AM, Ian Jackson wrote: > Boris Ostrovsky writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"): >> There are two interdependent variables that I need to print. The C >> equivalent is >> >> for ( i = 0; i < 4; i++ ) >> printf("%d %c\n", i, 'A'+i); >> >> The character value is derived from 'i', which in this example is an >> index into 'links' array. >> >> I suggested in response to Jan >> >> link=`echo "A B C D" | cut -d" " -f $i` > If the indices are necessarily successive integers: > > links="A B C D" > index=0 > for link in $links; do > index=$(( $index + 1 )) > something with $link and $index > > If the indices are arbitrary: > > links="1:A 4:B 7:C 10:D" > for linkinfo in $links; do > link=${linkinfo#*:} > index=${linkinfo%%:*} > something with $link and $index The indices are not successive, in one case they are a function of two enclosing loop indices, such as for dev in $(seq 1 31) do for intx in $(seq 0 3) do link_idx=$(((dev + intx) & 3)) printf " Package(){0x%04xffff, %u, \\\\_SB.PCI0.LNK%c, 0},\n" \ $dev $intx ${links:$link_idx:1} done done (And then there might also be a question of portability with the second approach?) So if you don't object to link=`echo "A B C D" | cut -d" " -f $i` I'd rather go with that. (I'll add '#!/bin/sh' as you requested in another email) -boris
Boris Ostrovsky writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"):
> On 09/26/2016 10:45 AM, Ian Jackson wrote:
> > If the indices are necessarily successive integers:
> >
> > links="A B C D"
> > index=0
> > for link in $links; do
> > index=$(( $index + 1 ))
> > something with $link and $index
> >
> > If the indices are arbitrary:
> >
> > links="1:A 4:B 7:C 10:D"
> > for linkinfo in $links; do
> > link=${linkinfo#*:}
> > index=${linkinfo%%:*}
> > something with $link and $index
>
>
> The indices are not successive, in one case they are a function of two
> enclosing loop indices, such as
By `indices' I meant the things which in your code are 1 2 3 4.
Apparently there is a different thing called an `idx'.
Your code below suggests that the numbers you need for each A B C D
are indeed successive integers.
> for dev in $(seq 1 31)
> do
> for intx in $(seq 0 3)
> do
> link_idx=$(((dev + intx) & 3))
> printf " Package(){0x%04xffff, %u, \\\\_SB.PCI0.LNK%c,
> 0},\n" \
> $dev $intx ${links:$link_idx:1}
> done
> done
>
> (And then there might also be a question of portability with the second
> approach?)
>
> So if you don't object to
>
> link=`echo "A B C D" | cut -d" " -f $i`
>
> I'd rather go with that.
I would still prefer
links="A B C D"
linkcounter=0
for link in $links
do
linkcounter=$(( $linkcounter + 1))
link_idx=$(( ($dev + $linkcounter) & 3 ))
I think this is a lot easier to read than messing about with seq and
string slicing.
NB that the spaces inside the $(( )) are IMO essential for
readability.
Ian.
On 09/26/2016 12:48 PM, Ian Jackson wrote: > Boris Ostrovsky writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"): >> On 09/26/2016 10:45 AM, Ian Jackson wrote: >>> If the indices are necessarily successive integers: >>> >>> links="A B C D" >>> index=0 >>> for link in $links; do >>> index=$(( $index + 1 )) >>> something with $link and $index >>> >>> If the indices are arbitrary: >>> >>> links="1:A 4:B 7:C 10:D" >>> for linkinfo in $links; do >>> link=${linkinfo#*:} >>> index=${linkinfo%%:*} >>> something with $link and $index >> >> The indices are not successive, in one case they are a function of two >> enclosing loop indices, such as > By `indices' I meant the things which in your code are 1 2 3 4. > Apparently there is a different thing called an `idx'. > > Your code below suggests that the numbers you need for each A B C D > are indeed successive integers. > >> for dev in $(seq 1 31) >> do >> for intx in $(seq 0 3) >> do >> link_idx=$(((dev + intx) & 3)) >> printf " Package(){0x%04xffff, %u, \\\\_SB.PCI0.LNK%c, >> 0},\n" \ >> $dev $intx ${links:$link_idx:1} >> done >> done >> >> (And then there might also be a question of portability with the second >> approach?) >> >> So if you don't object to >> >> link=`echo "A B C D" | cut -d" " -f $i` >> >> I'd rather go with that. > I would still prefer > > links="A B C D" > linkcounter=0 > for link in $links > do > linkcounter=$(( $linkcounter + 1)) > link_idx=$(( ($dev + $linkcounter) & 3 )) > This will not produce what I am trying to print tough. I want this sequence: B C D A C D A B D A B C A I can reverse loop order (I believe that even though resulting ASL will be different, the AML binary that iasl produces will stay the same) but then I think I will also need to reverse the dev loop direction. Or I am just not understanding what you are suggesting. -boris
Boris Ostrovsky writes ("Re: [PATCH v6] acpi: Prevent GPL-only code from seeping into non-GPL binaries"):
> This will not produce what I am trying to print tough. I want this sequence:
> B C D A C D A B D A B C A
I went back to your patch and AFAICT it doesn't generate that
sequence. But:
I think this conversation has turned into a ridiculous bikeshed
argument, for which I apologise. The code in your patch is acceptable
from a portability and style pov and I don't think we should spend any
more time trying to make it prettier.
Regards,
Ian.
diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile index 9f7357f..a1844d0 100644 --- a/tools/firmware/hvmloader/Makefile +++ b/tools/firmware/hvmloader/Makefile @@ -65,6 +65,9 @@ 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 +export GPL := y + .PHONY: all all: subdirs-all $(MAKE) hvmloader diff --git a/tools/firmware/hvmloader/acpi/Makefile b/tools/firmware/hvmloader/acpi/Makefile index f63e734..c23626d 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) + $(SHELL) 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) + $(SHELL) 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, 0x00010000) 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 0000000..28a0dd7 --- /dev/null +++ b/tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh @@ -0,0 +1,116 @@ +# 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/>. +# + +cat <<'EndOfASL' + /* Beginning of GPL-only code */ + + /* _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) + } +EndOfASL + +# PCI-ISA link definitions +# BUFA: List of ISA IRQs available for linking to PCI INTx. +# BUFB: IRQ descriptor for returning from link-device _CRS methods. +cat <<'EndOfASL' + Scope ( \_SB.PCI0 ) { + Name ( BUFA, ResourceTemplate() { IRQ(Level, ActiveLow, Shared) { 5, 10, 11 } } ) + Name ( BUFB, Buffer() { 0x23, 0x00, 0x00, 0x18, 0x79, 0 } ) + CreateWordField ( BUFB, 0x01, IRQV ) +EndOfASL + +links="ABCD" + +for i in $(seq 0 3) +do + link=${links:$i:1} + cat <<EndOfASL + Device ( LNK$link ) { + Name ( _HID, EISAID("PNP0C0F") ) + Name ( _UID, $((i+1))) + Method ( _STA, 0) { + If ( And(PIR$link, 0x80) ) { + Return ( 0x09 ) + } Else { + Return ( 0x0B ) + } + } + Method ( _PRS ) { + Return ( BUFA ) + } + Method ( _DIS ) { + Or ( PIR$link, 0x80, PIR$link ) + } + Method ( _CRS ) { + And ( PIR$link, 0x0f, Local0 ) + ShiftLeft ( 0x1, Local0, IRQV ) + Return ( BUFB ) + } + Method ( _SRS, 1 ) { + CreateWordField ( ARG0, 0x01, IRQ1 ) + FindSetRightBit ( IRQ1, Local0 ) + Decrement ( Local0 ) + Store ( Local0, PIR$link ) + } + } +EndOfASL +done + +# PCI interrupt routing definitions +# _PRT: Method to return routing table. +cat <<'EndOfASL' + Method ( _PRT, 0 ) { + If ( PICD ) { + Return ( PRTA ) + } + Return ( PRTP ) + } +EndOfASL + +# PRTP: PIC routing table (via ISA links). +echo " Name(PRTP, Package() {" +for dev in $(seq 1 31) +do + for intx in $(seq 0 3) # INTA-D + do + link_idx=$(((dev + intx) & 3)) + printf " Package(){0x%04xffff, %u, \\\\_SB.PCI0.LNK%c, 0},\n" \ + $dev $intx ${links:$link_idx:1} + done +done +echo " })" + +# PRTA: APIC routing table (via non-legacy IOAPIC GSIs). +echo " Name(PRTA, Package() {" +for dev in $(seq 1 31) +do + for intx in $(seq 0 3) # INTA-D + do + idx=$((((dev * 4 + dev/8 + intx) & 31) + 16)) + printf " Package(){0x%04xffff, %u, 0, %u},\n" \ + $dev $intx $idx + done +done +echo " })" + +echo " }" + +echo " /* End of GPL-only code */" diff --git a/tools/firmware/hvmloader/acpi/mk_dsdt.c b/tools/firmware/hvmloader/acpi/mk_dsdt.c index b2ade89..7656b5d 100644 --- a/tools/firmware/hvmloader/acpi/mk_dsdt.c +++ b/tools/firmware/hvmloader/acpi/mk_dsdt.c @@ -91,7 +91,7 @@ static struct option options[] = { int main(int argc, char **argv) { - unsigned int slot, dev, intx, link, cpu, max_cpus = HVM_MAX_VCPUS; + unsigned int slot, cpu, max_cpus = HVM_MAX_VCPUS; dm_version dm_version = QEMU_XEN_TRADITIONAL; for ( ; ; ) @@ -273,72 +273,6 @@ int main(int argc, char **argv) } } pop_block(); - /*** PCI-ISA link definitions ***/ - /* BUFA: List of ISA IRQs available for linking to PCI INTx. */ - stmt("Name", "BUFA, ResourceTemplate() { " - "IRQ(Level, ActiveLow, Shared) { 5, 10, 11 } }"); - /* BUFB: IRQ descriptor for returning from link-device _CRS methods. */ - stmt("Name", "BUFB, Buffer() { " - "0x23, 0x00, 0x00, 0x18, " /* IRQ descriptor */ - "0x79, 0 }"); /* End tag, null checksum */ - stmt("CreateWordField", "BUFB, 0x01, IRQV"); - /* Create four PCI-ISA link devices: LNKA, LNKB, LNKC, LNKD. */ - for ( link = 0; link < 4; link++ ) - { - push_block("Device", "LNK%c", 'A'+link); - stmt("Name", "_HID, EISAID(\"PNP0C0F\")"); /* PCI interrupt link */ - stmt("Name", "_UID, %u", link+1); - push_block("Method", "_STA, 0"); - push_block("If", "And(PIR%c, 0x80)", 'A'+link); - stmt("Return", "0x09"); - pop_block(); - push_block("Else", NULL); - stmt("Return", "0x0B"); - pop_block(); - pop_block(); - push_block("Method", "_PRS"); - stmt("Return", "BUFA"); - pop_block(); - push_block("Method", "_DIS"); - stmt("Or", "PIR%c, 0x80, PIR%c", 'A'+link, 'A'+link); - pop_block(); - push_block("Method", "_CRS"); - stmt("And", "PIR%c, 0x0f, Local0", 'A'+link); - stmt("ShiftLeft", "0x1, Local0, IRQV"); - stmt("Return", "BUFB"); - pop_block(); - push_block("Method", "_SRS, 1"); - stmt("CreateWordField", "ARG0, 0x01, IRQ1"); - stmt("FindSetRightBit", "IRQ1, Local0"); - stmt("Decrement", "Local0"); - stmt("Store", "Local0, PIR%c", 'A'+link); - pop_block(); - pop_block(); - } - - /*** PCI interrupt routing definitions***/ - /* _PRT: Method to return routing table. */ - push_block("Method", "_PRT, 0"); - push_block("If", "PICD"); - stmt("Return", "PRTA"); - pop_block(); - stmt("Return", "PRTP"); - pop_block(); - /* PRTP: PIC routing table (via ISA links). */ - printf("Name(PRTP, Package() {\n"); - for ( dev = 1; dev < 32; dev++ ) - for ( intx = 0; intx < 4; intx++ ) /* INTA-D */ - printf("Package(){0x%04xffff, %u, \\_SB.PCI0.LNK%c, 0},\n", - dev, intx, 'A'+((dev+intx)&3)); - printf("})\n"); - /* PRTA: APIC routing table (via non-legacy IOAPIC GSIs). */ - printf("Name(PRTA, Package() {\n"); - for ( dev = 1; dev < 32; dev++ ) - for ( intx = 0; intx < 4; intx++ ) /* INTA-D */ - printf("Package(){0x%04xffff, %u, 0, %u},\n", - dev, intx, ((dev*4+dev/8+intx)&31)+16); - printf("})\n"); - /* * Each PCI hotplug slot needs at least two methods to handle * the ACPI event:
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 <boris.ostrovsky@oracle.com> --- Changes in v6: * Replaced script's printf in most case with "here document" (for multi-line text) or echo for single line. Left printf for formatted output. (Note that in one case paramter expansion is necessary and so delimiter word is intentionally not quoted). * Replaced bash arrays with ${string:index:size} syntax. * Style adjustments as requested by Jan. I am sending only this patch from series. A couple of patches are affected by changes here. I will re-send the whole series if needed. tools/firmware/hvmloader/Makefile | 3 + tools/firmware/hvmloader/acpi/Makefile | 14 ++- tools/firmware/hvmloader/acpi/dsdt.asl | 14 --- tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh | 116 +++++++++++++++++++++++ tools/firmware/hvmloader/acpi/mk_dsdt.c | 68 +------------ 5 files changed, 131 insertions(+), 84 deletions(-) create mode 100755 tools/firmware/hvmloader/acpi/gpl/mk_dsdt_gpl.sh