[v1,02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code

Commit Message

Boris Ostrovsky July 5, 2016, 7:05 p.m. UTC

acpi_info can be initialized by hvmloader itself. Now ACPI code
doesn't need to use hvmloader-private variables/routines such as
uart_exists(), lpt_exists() etc.

Signed-off-by: Boris Ostrovsky <[e-mail]>
---
Changes in v1:
* Create libacpi.h for libacpi interface definitions
* Move RSDP struct acpi_info pointers into struct acpi_config 

 tools/firmware/hvmloader/acpi/acpi2_0.h |   11 ----
 tools/firmware/hvmloader/acpi/build.c   |   48 +++---------------
 tools/firmware/hvmloader/acpi/libacpi.h |   80 +++++++++++++++++++++++++++++++
 tools/firmware/hvmloader/seabios.c      |    1 +
 tools/firmware/hvmloader/util.c         |   21 ++++++++-
 5 files changed, 109 insertions(+), 52 deletions(-)
 create mode 100644 tools/firmware/hvmloader/acpi/libacpi.h

Comments

Ian Jackson July 7, 2016, 4:58 p.m. UTC | #1

Boris Ostrovsky writes ("[PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> acpi_info can be initialized by hvmloader itself. Now ACPI code
> doesn't need to use hvmloader-private variables/routines such as
> uart_exists(), lpt_exists() etc.
...
> Signed-off-by: Boris Ostrovsky <[e-mail]>
...
> * Create libacpi.h for libacpi interface definitions

Nacked-by: Ian Jackson <[e-mail]>

I'm afraid this patch contains a licence violation, contrary to your
S-O-B.


You have moved this:

> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
...
> -struct acpi_info {
> -    uint8_t  com1_present:1;    /* 0[0] - System has COM1? */
> -    uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
> -    uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */


Into a new file:

> --- /dev/null
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -0,0 +1,80 @@
...
> +struct acpi_info {
> +    uint8_t  com1_present:1;    /* 0[0] - System has COM1? */
> +    uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
> +    uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */


The original file has this copyright header:

  /*
   * Copyright (c) 2004, Intel Corporation.
   * Copyright (c) 2006, Keir Fraser, XenSource Inc.
   *
   * 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/>.
   */

(rewrapped for legibility).  The dates haven't been updated for ages,
but this file is clearly GPLv2-only.


However your new file has this copyright header (again, wrapped);

> +/******************************************************************************
> + * libacpi.h
> + * 
> + * libacpi interfaces
> + * 
> + * Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */

There are two serious problems with this.

1. You have dropped the copyright attribution to Intel and Xensource.

2. You have changed the licence to BSD-style, even though the original
   was GPLv2-only.


Please be more careful!

Thanks,
Ian.

Boris Ostrovsky July 7, 2016, 5:09 p.m. UTC | #2

On 07/07/2016 12:58 PM, Ian Jackson wrote:
>
> There are two serious problems with this.
>
> 1. You have dropped the copyright attribution to Intel and Xensource.
>
> 2. You have changed the licence to BSD-style, even though the original
>    was GPLv2-only.


I meant this to be a GPLv2 license. And I made the same mistake in a
couple of other files.

-boris


>
>
> Please be more careful!
>
> Thanks,
> Ian.

Wei Liu July 7, 2016, 5:15 p.m. UTC | #3

On Thu, Jul 07, 2016 at 01:09:28PM -0400, Boris Ostrovsky wrote:
> On 07/07/2016 12:58 PM, Ian Jackson wrote:
> >
> > There are two serious problems with this.
> >
> > 1. You have dropped the copyright attribution to Intel and Xensource.
> >
> > 2. You have changed the licence to BSD-style, even though the original
> >    was GPLv2-only.
> 
> 
> I meant this to be a GPLv2 license. And I made the same mistake in a
> couple of other files.
> 

The other question is, will this GPLv2 file be linked against other
toolstack components (libxl is LGPL)? If so, how should we deal with
different licences?

Wei.

> -boris
> 
> 
> >
> >
> > Please be more careful!
> >
> > Thanks,
> > Ian.
> 
>

Boris Ostrovsky July 7, 2016, 5:45 p.m. UTC | #4

On 07/07/2016 01:15 PM, Wei Liu wrote:
> On Thu, Jul 07, 2016 at 01:09:28PM -0400, Boris Ostrovsky wrote:
>> On 07/07/2016 12:58 PM, Ian Jackson wrote:
>>> There are two serious problems with this.
>>>
>>> 1. You have dropped the copyright attribution to Intel and Xensource.
>>>
>>> 2. You have changed the licence to BSD-style, even though the original
>>>    was GPLv2-only.
>>
>> I meant this to be a GPLv2 license. And I made the same mistake in a
>> couple of other files.
>>
> The other question is, will this GPLv2 file be linked against other
> toolstack components (libxl is LGPL)? If so, how should we deal with
> different licences?

Two new libxl files will be LGPL and but libacpi files will be GPLv2
(because most of the files there are already GPLv2, I just added a
couple of new ones).

Which would mean that a non-GPL users cannot link against libxl anymore,
right?

-boris

>
> Wei.
>
>> -boris
>>
>>
>>>
>>> Please be more careful!
>>>
>>> Thanks,
>>> Ian.
>>

Jan Beulich July 8, 2016, 10:10 a.m. UTC | #5

>>> On 05.07.16 at 21:05, <[e-mail]> wrote:
> --- a/tools/firmware/hvmloader/acpi/acpi2_0.h
> +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
> @@ -449,17 +449,6 @@ struct acpi_20_slit {
>  #define ACPI_2_0_SRAT_REVISION 0x01
>  #define ACPI_2_0_SLIT_REVISION 0x01
>  
> -#pragma pack ()

This must not be removed from here.

> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, unsigned int physical)
>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>                   sizeof(struct acpi_20_rsdp));
>  
> -    if ( !new_vm_gid(acpi_info) )
> +    if ( !new_vm_gid(&config->ainfo) )
>          goto oom;
>  
> -    acpi_info->com1_present = uart_exists(0x3f8);
> -    acpi_info->com2_present = uart_exists(0x2f8);
> -    acpi_info->lpt1_present = lpt_exists(0x378);
> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
> -    acpi_info->pci_min = pci_mem_start;
> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
> -    if ( pci_hi_mem_end > pci_hi_mem_start )
> -    {
> -        acpi_info->pci_hi_min = pci_hi_mem_start;
> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
> -    }
> +    *(struct acpi_info *)config->ainfop = config->ainfo;

With your new separation of responsibilities - does this really
belong here rather than in the caller? And if it is to stay here, then
I think the pointer field should be made of pointer type, leaving it
to the caller to do whatever casting is necessary when filling in that
field.

> --- /dev/null
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -0,0 +1,80 @@
> +/******************************************************************************
> + * libacpi.h
> + * 
> + * libacpi interfaces
> + * 
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +
> +#ifndef __LIBACPI_H__
> +#define __LIBACPI_H__
> +
> +#pragma pack ()

I guess this is going to be unneeded once the removal from acpi2_0.h
gets undone?

> +/*
> + * This must match the Field("BIOS"....) definition in the DSDT.
> + */

This is a single line comment. And considering what this comment
says I wonder whether the following structure definition wouldn't
better be wrapped in a #pragma pack(1) / #pragma pack() pair,
with ...

> +struct acpi_info {
> +    uint8_t  com1_present:1;    /* 0[0] - System has COM1? */
> +    uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
> +    uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
> +    uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */

explicit (unnamed) padding added here.

> +struct acpi_config {
> +    const unsigned char *dsdt_anycpu;
> +    unsigned int dsdt_anycpu_len;
> +    const unsigned char *dsdt_15cpu;
> +    unsigned int dsdt_15cpu_len;
> +
> +    /* May have some fields updated by acpi_build_table() */
> +    struct acpi_info ainfo;
> +    /*
> +     * Address where acpi_info should be placed.
> +     * This must match the OperationRegion(BIOS, SystemMemory, ....)
> +     * definition in the DSDT
> +     */
> +    unsigned int ainfop;

What is the "a" prefix of these two fields good for? Considering the
name of the structure I don't see a need for any prefix. But if you
absolutely want one, then acpi_ please.

Jan

Boris Ostrovsky July 8, 2016, 2:39 p.m. UTC | #6

On 07/08/2016 06:10 AM, Jan Beulich wrote:
>
>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, unsigned int physical)
>>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>>                   sizeof(struct acpi_20_rsdp));
>>  
>> -    if ( !new_vm_gid(acpi_info) )
>> +    if ( !new_vm_gid(&config->ainfo) )
>>          goto oom;
>>  
>> -    acpi_info->com1_present = uart_exists(0x3f8);
>> -    acpi_info->com2_present = uart_exists(0x2f8);
>> -    acpi_info->lpt1_present = lpt_exists(0x378);
>> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>> -    acpi_info->pci_min = pci_mem_start;
>> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
>> -    if ( pci_hi_mem_end > pci_hi_mem_start )
>> -    {
>> -        acpi_info->pci_hi_min = pci_hi_mem_start;
>> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>> -    }
>> +    *(struct acpi_info *)config->ainfop = config->ainfo;
> With your new separation of responsibilities - does this really
> belong here rather than in the caller? 

I think it should be done here: when the call returns all tables are
already in memory. If the caller wants to load those tables separately
(as probably the toolstack will) then it can simply copy it as a blob.

>
>> +struct acpi_config {
>> +    const unsigned char *dsdt_anycpu;
>> +    unsigned int dsdt_anycpu_len;
>> +    const unsigned char *dsdt_15cpu;
>> +    unsigned int dsdt_15cpu_len;
>> +
>> +    /* May have some fields updated by acpi_build_table() */
>> +    struct acpi_info ainfo;
>> +    /*
>> +     * Address where acpi_info should be placed.
>> +     * This must match the OperationRegion(BIOS, SystemMemory, ....)
>> +     * definition in the DSDT
>> +     */
>> +    unsigned int ainfop;
> What is the "a" prefix of these two fields good for? Considering the
> name of the structure I don't see a need for any prefix. But if you
> absolutely want one, then acpi_ please.

You requested 'acpi_' prefix to be dropped earlier. I added 'a' (and
'pt_' in patch 5) to be a bit more cscope-friendly: names like info,
addr and length will result in lots of unnecessary hits.

-boris

Konrad Rzeszutek Wilk July 8, 2016, 3:06 p.m. UTC | #7

On Thu, Jul 07, 2016 at 01:45:05PM -0400, Boris Ostrovsky wrote:
> On 07/07/2016 01:15 PM, Wei Liu wrote:
> > On Thu, Jul 07, 2016 at 01:09:28PM -0400, Boris Ostrovsky wrote:
> >> On 07/07/2016 12:58 PM, Ian Jackson wrote:
> >>> There are two serious problems with this.
> >>>
> >>> 1. You have dropped the copyright attribution to Intel and Xensource.
> >>>
> >>> 2. You have changed the licence to BSD-style, even though the original
> >>>    was GPLv2-only.
> >>
> >> I meant this to be a GPLv2 license. And I made the same mistake in a
> >> couple of other files.
> >>
> > The other question is, will this GPLv2 file be linked against other
> > toolstack components (libxl is LGPL)? If so, how should we deal with
> > different licences?
> 
> Two new libxl files will be LGPL and but libacpi files will be GPLv2
> (because most of the files there are already GPLv2, I just added a
> couple of new ones).
> 
> Which would mean that a non-GPL users cannot link against libxl anymore,
> right?

Having different licenses will invite the lawyers in the conversation
which can drag things out.

A quick read says one can add an exception to GPLv2 license to allow it
to be linked (see https://www.gnu.org/licenses/gpl-faq.en.html#GPLIncompatibleLibs)
but that would require Copyright OK from the original holders.

It would be far easier to ask the copyright holders:

Andrew Cooper <[e-mail]>
 Anthony PERARD <[e-mail]>
 David Vrabel <[e-mail]>
 Dexuan Cui <[e-mail]>
 Eddie Dong <[e-mail]>
 Ian Campbell <[e-mail]>
 John Levon <[e-mail]>
 Keir Fraser <[e-mail]>
 Keir Fraser <[e-mail]>
 Liu, Jinsong <[e-mail]>
 Paul Durrant <[e-mail]>
 Qing He <[e-mail]>
 Stefan Berger <[e-mail]>
 Tim Deegan <[e-mail]>
 Tobias Geiger <[e-mail]>
 Xiaowei Yang <[e-mail]>

If they would be OK making the code (this is from
tools/firmware/hvmloader/acpi/acpi2_0.h) lGPL.

Or is there some other technical way around this?

I can't recall whether the 'dlopen' (so runtime loading
vs linking) of an GPL library is from Lesser GPL is OK.
(so proprietary code linking with libxl, and libxl dlopen'ing
the libacpi code').

Sounds like we may need to consult the lawyers after all.

Jan Beulich July 8, 2016, 3:11 p.m. UTC | #8

>>> On 08.07.16 at 16:39, <[e-mail]> wrote:
> On 07/08/2016 06:10 AM, Jan Beulich wrote:
>>
>>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, 
> unsigned int physical)
>>>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>>>                   sizeof(struct acpi_20_rsdp));
>>>  
>>> -    if ( !new_vm_gid(acpi_info) )
>>> +    if ( !new_vm_gid(&config->ainfo) )
>>>          goto oom;
>>>  
>>> -    acpi_info->com1_present = uart_exists(0x3f8);
>>> -    acpi_info->com2_present = uart_exists(0x2f8);
>>> -    acpi_info->lpt1_present = lpt_exists(0x378);
>>> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>>> -    acpi_info->pci_min = pci_mem_start;
>>> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
>>> -    if ( pci_hi_mem_end > pci_hi_mem_start )
>>> -    {
>>> -        acpi_info->pci_hi_min = pci_hi_mem_start;
>>> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>>> -    }
>>> +    *(struct acpi_info *)config->ainfop = config->ainfo;
>> With your new separation of responsibilities - does this really
>> belong here rather than in the caller? 
> 
> I think it should be done here: when the call returns all tables are
> already in memory. If the caller wants to load those tables separately
> (as probably the toolstack will) then it can simply copy it as a blob.

But this structure isn't part of the ACPI tables, and by not doing
it here (a) at least some of the intended callers may be able to
get away without the ugly cast and (b) the field now named
ainfop wouldn't be needed either afaict.

>>> +struct acpi_config {
>>> +    const unsigned char *dsdt_anycpu;
>>> +    unsigned int dsdt_anycpu_len;
>>> +    const unsigned char *dsdt_15cpu;
>>> +    unsigned int dsdt_15cpu_len;
>>> +
>>> +    /* May have some fields updated by acpi_build_table() */
>>> +    struct acpi_info ainfo;
>>> +    /*
>>> +     * Address where acpi_info should be placed.
>>> +     * This must match the OperationRegion(BIOS, SystemMemory, ....)
>>> +     * definition in the DSDT
>>> +     */
>>> +    unsigned int ainfop;
>> What is the "a" prefix of these two fields good for? Considering the
>> name of the structure I don't see a need for any prefix. But if you
>> absolutely want one, then acpi_ please.
> 
> You requested 'acpi_' prefix to be dropped earlier. I added 'a' (and
> 'pt_' in patch 5) to be a bit more cscope-friendly: names like info,
> addr and length will result in lots of unnecessary hits.

I remember having that discussion once with Mukesh too. I continue
to think source code should not get uglified just because of some
random tool's limitations, maybe unless _everyone_ uses that tool.

Jan

Ian Jackson July 8, 2016, 3:50 p.m. UTC | #9

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> Having different licenses will invite the lawyers in the conversation
> which can drag things out.

We don't want libxl to have some confusing combination of
alleged-licences.

> A quick read says one can add an exception to GPLv2 license to allow it
> to be linked (see https://www.gnu.org/licenses/gpl-faq.en.html#GPLIncompatibleLibs)
> but that would require Copyright OK from the original holders.
> 
> It would be far easier to ask the copyright holders:

Yes.  That seems to be Citrix, Intel, Sun (Oracle), IBM, and:

>  Tobias Geiger <[e-mail]>
> 
> If they would be OK making the code (this is from
> tools/firmware/hvmloader/acpi/acpi2_0.h) lGPL.

Right.

> Or is there some other technical way around this?

No.

> I can't recall whether the 'dlopen' (so runtime loading
> vs linking) of an GPL library is from Lesser GPL is OK.
> (so proprietary code linking with libxl, and libxl dlopen'ing
> the libacpi code').

This kind of attempt at licence workaround by some kind of technical
bodge is not legally effective.

Ian.

Boris Ostrovsky July 8, 2016, 3:57 p.m. UTC | #10

On 07/08/2016 11:50 AM, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
>> Having different licenses will invite the lawyers in the conversation
>> which can drag things out.
> We don't want libxl to have some confusing combination of
> alleged-licences.
>
>> A quick read says one can add an exception to GPLv2 license to allow it
>> to be linked (see https://www.gnu.org/licenses/gpl-faq.en.html#GPLIncompatibleLibs)
>> but that would require Copyright OK from the original holders.
>>
>> It would be far easier to ask the copyright holders:
> Yes.  That seems to be Citrix, Intel, Sun (Oracle), IBM, and:
>
>>  Tobias Geiger <[e-mail]>


Do we need to get consent from companies only or (also) from individuals
listed in the files? Which means Keir and Kamala Narasimhan (who works,
or at least used to work) for Citrix.

For IBM --- whom can we contact?

>>
>> If they would be OK making the code (this is from
>> tools/firmware/hvmloader/acpi/acpi2_0.h) lGPL.
> Right.
>
>> Or is there some other technical way around this?
> No.
>
>> I can't recall whether the 'dlopen' (so runtime loading
>> vs linking) of an GPL library is from Lesser GPL is OK.
>> (so proprietary code linking with libxl, and libxl dlopen'ing
>> the libacpi code').
> This kind of attempt at licence workaround by some kind of technical
> bodge is not legally effective.

We don't build files in libacpi as a dynamic library. The object files
are linked against whoever wants to use the functionality, just like
what we do for libelf.

-boris

Boris Ostrovsky July 8, 2016, 4:14 p.m. UTC | #11

On 07/08/2016 11:11 AM, Jan Beulich wrote:
>>>> On 08.07.16 at 16:39, <[e-mail]> wrote:
>> On 07/08/2016 06:10 AM, Jan Beulich wrote:
>>>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, 
>> unsigned int physical)
>>>>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>>>>                   sizeof(struct acpi_20_rsdp));
>>>>  
>>>> -    if ( !new_vm_gid(acpi_info) )
>>>> +    if ( !new_vm_gid(&config->ainfo) )
>>>>          goto oom;
>>>>  
>>>> -    acpi_info->com1_present = uart_exists(0x3f8);
>>>> -    acpi_info->com2_present = uart_exists(0x2f8);
>>>> -    acpi_info->lpt1_present = lpt_exists(0x378);
>>>> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>>>> -    acpi_info->pci_min = pci_mem_start;
>>>> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
>>>> -    if ( pci_hi_mem_end > pci_hi_mem_start )
>>>> -    {
>>>> -        acpi_info->pci_hi_min = pci_hi_mem_start;
>>>> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>>>> -    }
>>>> +    *(struct acpi_info *)config->ainfop = config->ainfo;
>>> With your new separation of responsibilities - does this really
>>> belong here rather than in the caller? 
>> I think it should be done here: when the call returns all tables are
>> already in memory. If the caller wants to load those tables separately
>> (as probably the toolstack will) then it can simply copy it as a blob.
> But this structure isn't part of the ACPI tables, and by not doing
> it here (a) at least some of the intended callers may be able to
> get away without the ugly cast and (b) the field now named
> ainfop wouldn't be needed either afaict.


I probably didn't use right terminology. This is not a table, but an AML
piece? In any case, it's something that is ACPI-specific and I was
hoping we wouldn't need to expose this to the caller. The fact that it
is passed in the right format in struct acpi_info is a happy
coincidence. We may change it in the future (and so perhaps I should
drop the comment in libacpi.h about "This must match the
Field("BIOS"....) definition in the DSDT.")

-boris

Ian Jackson July 8, 2016, 4:21 p.m. UTC | #12

Boris Ostrovsky writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> Do we need to get consent from companies only or (also) from individuals
> listed in the files? Which means Keir and Kamala Narasimhan (who works,
> or at least used to work) for Citrix.

Where the people were doing the work as part of their employemnt we
need consent only from their employer.

I confess I didn't check the dates of Keir's contributions but they
probably postdate his employment by XenSource.

> For IBM --- whom can we contact?

The named person is probably the first point of contact.

Ian.

Wei Liu July 11, 2016, 12:10 p.m. UTC | #13

CC Lars

More licence stuff. Do you have contact(s)?

Wei.

On Fri, Jul 08, 2016 at 11:57:59AM -0400, Boris Ostrovsky wrote:
> On 07/08/2016 11:50 AM, Ian Jackson wrote:
> > Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> >> Having different licenses will invite the lawyers in the conversation
> >> which can drag things out.
> > We don't want libxl to have some confusing combination of
> > alleged-licences.
> >
> >> A quick read says one can add an exception to GPLv2 license to allow it
> >> to be linked (see https://www.gnu.org/licenses/gpl-faq.en.html#GPLIncompatibleLibs)
> >> but that would require Copyright OK from the original holders.
> >>
> >> It would be far easier to ask the copyright holders:
> > Yes.  That seems to be Citrix, Intel, Sun (Oracle), IBM, and:
> >
> >>  Tobias Geiger <[e-mail]>
> 
> 
> Do we need to get consent from companies only or (also) from individuals
> listed in the files? Which means Keir and Kamala Narasimhan (who works,
> or at least used to work) for Citrix.
> 
> For IBM --- whom can we contact?
> 
> >>
> >> If they would be OK making the code (this is from
> >> tools/firmware/hvmloader/acpi/acpi2_0.h) lGPL.
> > Right.
> >
> >> Or is there some other technical way around this?
> > No.
> >
> >> I can't recall whether the 'dlopen' (so runtime loading
> >> vs linking) of an GPL library is from Lesser GPL is OK.
> >> (so proprietary code linking with libxl, and libxl dlopen'ing
> >> the libacpi code').
> > This kind of attempt at licence workaround by some kind of technical
> > bodge is not legally effective.
> 
> We don't build files in libacpi as a dynamic library. The object files
> are linked against whoever wants to use the functionality, just like
> what we do for libelf.
> 
> -boris
>

Lars Kurth July 11, 2016, 2:47 p.m. UTC | #14

> On 11 Jul 2016, at 13:10, Wei Liu <[e-mail]> wrote:
> 
> CC Lars
> 
> More licence stuff. Do you have contact(s)?

James (on vacation) or I can confirm on behalf of Citrix.
I am assuming that Konrad can get the approval for Sun/Oracle
As for Intel, we can ask Susie Li. It would have to be a Intel decision because many of the people on that list are not working for Intel anymore.

We should probably double check the dates of Keir's contributions: getting his explicit ACK is most likely not an issue
 
According to LinkedIn, Stefan Berger still works for IBM.
And a quick google search for Tobias Geiger's e-mail address and 2016 shows that the e-mail address was last used a month ago. 

> It would be far easier to ask the copyright holders:
> 
> Andrew Cooper <[e-mail]>
> Anthony PERARD <[e-mail]>
> David Vrabel <[e-mail]>
> Dexuan Cui <[e-mail]>
> Eddie Dong <[e-mail]>
> Ian Campbell <[e-mail]>
> John Levon <[e-mail]>
> Keir Fraser <[e-mail]>
> Keir Fraser <[e-mail]>
> Liu, Jinsong <[e-mail]>
> Paul Durrant <[e-mail]>
> Qing He <[e-mail]>
> Stefan Berger <[e-mail]>
> Tim Deegan <[e-mail]>
> Tobias Geiger <[e-mail]>
> Xiaowei Yang <[e-mail]>

How was this list established? I get a different set of names for both
build.c   http://xenbits.xen.org/gitweb/?p=xen.git;a=history;f=tools/firmware/hvmloader/acpi/build.c
acpi2_0.h http://xenbits.xen.org/gitweb/?p=xen.git;a=history;f=tools/firmware/hvmloader/acpi/acpi2_0.h

Or did I miss anything?

Regards
Lars

Konrad Rzeszutek Wilk July 11, 2016, 2:54 p.m. UTC | #15

On Mon, Jul 11, 2016 at 03:47:20PM +0100, Lars Kurth wrote:
> 
> > On 11 Jul 2016, at 13:10, Wei Liu <[e-mail]> wrote:
> > 
> > CC Lars
> > 
> > More licence stuff. Do you have contact(s)?
> 
> James (on vacation) or I can confirm on behalf of Citrix.
> I am assuming that Konrad can get the approval for Sun/Oracle
> As for Intel, we can ask Susie Li. It would have to be a Intel decision because many of the people on that list are not working for Intel anymore.
> 
> We should probably double check the dates of Keir's contributions: getting his explicit ACK is most likely not an issue
>  
> According to LinkedIn, Stefan Berger still works for IBM.
> And a quick google search for Tobias Geiger's e-mail address and 2016 shows that the e-mail address was last used a month ago. 
> 
> > It would be far easier to ask the copyright holders:
> > 
> > Andrew Cooper <[e-mail]>
> > Anthony PERARD <[e-mail]>
> > David Vrabel <[e-mail]>
> > Dexuan Cui <[e-mail]>
> > Eddie Dong <[e-mail]>
> > Ian Campbell <[e-mail]>
> > John Levon <[e-mail]>
> > Keir Fraser <[e-mail]>
> > Keir Fraser <[e-mail]>
> > Liu, Jinsong <[e-mail]>
> > Paul Durrant <[e-mail]>
> > Qing He <[e-mail]>
> > Stefan Berger <[e-mail]>
> > Tim Deegan <[e-mail]>
> > Tobias Geiger <[e-mail]>
> > Xiaowei Yang <[e-mail]>
> 
> How was this list established? I get a different set of names for both
> build.c   http://xenbits.xen.org/gitweb/?p=xen.git;a=history;f=tools/firmware/hvmloader/acpi/build.c
> acpi2_0.h http://xenbits.xen.org/gitweb/?p=xen.git;a=history;f=tools/firmware/hvmloader/acpi/acpi2_0.h
> 
> Or did I miss anything?

I did:

git log tools/firmware/hvmloader/acpi/acpi2_0.h  | grep \@ | sed s/.*:// | sort | uniq
And removed the duplicate entries.

But hadn't done the build.c which has:

[konrad@char xen]$  git log tools/firmware/hvmloader/acpi/build.c  | grep \@ | sed s/.*:// | sort | uniq
 Adnan Misherfi <[e-mail]>
 Andrew Cooper <[e-mail]>
 Anthony PERARD <[e-mail]>
 Atsushi SAKAI <[e-mail]>
 Christoph Egger <[e-mail]>
 Dan Magenheimer <[e-mail]>
 David Vrabel <[e-mail]
 David Vrabel <[e-mail]>
 Dexuan Cui <[e-mail]>
 Eddie Dong <[e-mail]>
 Frediano Ziglio <[e-mail]>
 Ian Campbell <[e-mail]>
 Jan Beulich <[e-mail]>
 [e-mail] <[e-mail]>
 Kamala Narasimhan <[e-mail]>
 Keir Fraser <[e-mail]>
 Keir Fraser <[e-mail]>
 Keir Fraser <[e-mail]>
 Keir Fraser <[e-mail]>
 [e-mail] <[e-mail]>
 Konrad Rzeszutek Wilk <[e-mail]>
 Liu, Jinsong <[e-mail]>
 Paolo Bonzini <[e-mail]>
 Paul Durrant <[e-mail]>
 Qing He <[e-mail]>
 Ross Philipson <[e-mail]>
 Samuel Thibault <[e-mail]>
 Trolle Selander <[e-mail]>
 Xiaowei Yang <[e-mail]>


Thought http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=883236e49a86a0174c6df61cac995ebf16d72b35
shows that the file was moved, and I hadn't taken that into account.

So would need to look even further back under a different directory.

> 
> Regards
> Lars

Boris Ostrovsky July 11, 2016, 3:06 p.m. UTC | #16

On 07/11/2016 10:54 AM, Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 11, 2016 at 03:47:20PM +0100, Lars Kurth wrote:
>>> On 11 Jul 2016, at 13:10, Wei Liu <[e-mail]> wrote:
>>>
>>> CC Lars
>>>
>>> More licence stuff. Do you have contact(s)?
>> James (on vacation) or I can confirm on behalf of Citrix.
>> I am assuming that Konrad can get the approval for Sun/Oracle
>> As for Intel, we can ask Susie Li. It would have to be a Intel decision because many of the people on that list are not working for Intel anymore.
>>
>> We should probably double check the dates of Keir's contributions: getting his explicit ACK is most likely not an issue
>>  
>> According to LinkedIn, Stefan Berger still works for IBM.
>> And a quick google search for Tobias Geiger's e-mail address and 2016 shows that the e-mail address was last used a month ago. 
>>
>>> It would be far easier to ask the copyright holders:
>>>
>>> Andrew Cooper <[e-mail]>
>>> Anthony PERARD <[e-mail]>
>>> David Vrabel <[e-mail]>
>>> Dexuan Cui <[e-mail]>
>>> Eddie Dong <[e-mail]>
>>> Ian Campbell <[e-mail]>
>>> John Levon <[e-mail]>
>>> Keir Fraser <[e-mail]>
>>> Keir Fraser <[e-mail]>
>>> Liu, Jinsong <[e-mail]>
>>> Paul Durrant <[e-mail]>
>>> Qing He <[e-mail]>
>>> Stefan Berger <[e-mail]>
>>> Tim Deegan <[e-mail]>
>>> Tobias Geiger <[e-mail]>
>>> Xiaowei Yang <[e-mail]>
>> How was this list established? I get a different set of names for both
>> build.c   http://xenbits.xen.org/gitweb/?p=xen.git;a=history;f=tools/firmware/hvmloader/acpi/build.c
>> acpi2_0.h http://xenbits.xen.org/gitweb/?p=xen.git;a=history;f=tools/firmware/hvmloader/acpi/acpi2_0.h
>>
>> Or did I miss anything?
> I did:
>
> git log tools/firmware/hvmloader/acpi/acpi2_0.h  | grep \@ | sed s/.*:// | sort | uniq
> And removed the duplicate entries.
>
> But hadn't done the build.c which has:
>
> [konrad@char xen]$  git log tools/firmware/hvmloader/acpi/build.c  | grep \@ | sed s/.*:// | sort | uniq
>  Adnan Misherfi <[e-mail]>
>  Andrew Cooper <[e-mail]>
>  Anthony PERARD <[e-mail]>
>  Atsushi SAKAI <[e-mail]>
>  Christoph Egger <[e-mail]>
>  Dan Magenheimer <[e-mail]>
>  David Vrabel <[e-mail]
>  David Vrabel <[e-mail]>
>  Dexuan Cui <[e-mail]>
>  Eddie Dong <[e-mail]>
>  Frediano Ziglio <[e-mail]>
>  Ian Campbell <[e-mail]>
>  Jan Beulich <[e-mail]>
>  [e-mail] <[e-mail]>
>  Kamala Narasimhan <[e-mail]>
>  Keir Fraser <[e-mail]>
>  Keir Fraser <[e-mail]>
>  Keir Fraser <[e-mail]>
>  Keir Fraser <[e-mail]>
>  [e-mail] <[e-mail]>
>  Konrad Rzeszutek Wilk <[e-mail]>
>  Liu, Jinsong <[e-mail]>
>  Paolo Bonzini <[e-mail]>
>  Paul Durrant <[e-mail]>
>  Qing He <[e-mail]>
>  Ross Philipson <[e-mail]>
>  Samuel Thibault <[e-mail]>
>  Trolle Selander <[e-mail]>
>  Xiaowei Yang <[e-mail]>
>
>
> Thought http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=883236e49a86a0174c6df61cac995ebf16d72b35
> shows that the file was moved, and I hadn't taken that into account.
>
> So would need to look even further back under a different directory.


But do we need to look at who touched the file or who is explicitly
listed as copyright holder (in files' headers)?

So I did (because I thought only Signed-off might important, is it?)

ostr@workbase> git log . | grep Signed | sed -e 's/.*@/ /g' | sort | uniq
 citrix.com
 citrix.com>
 debian.org>
 eu.citrix.com>
 intel.com>
 jp.fujitsu.com>
 net-space.pl>
 novell.com>
 oracle.com>
 redhat.com>
 sun.com>
 suse.com>
 us.ibm.com>
 verge.net.au>
 xen.org>
 xensource.com>

-boris

Ian Jackson July 11, 2016, 3:38 p.m. UTC | #17

Boris Ostrovsky writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> But do we need to look at who touched the file or who is explicitly
> listed as copyright holder (in files' headers)?

You need to look at the history.

Sadly, we don't have any means of keeping the copyright headers up to
date with lists of contributors.

> So I did (because I thought only Signed-off might important, is it?)

No.  In general I think From: is probably more relevant but I would
include both From: and S-o-b:.

If there are edge cases that crop up only once or twice they could be
excluded.

> ostr@workbase> git log . | grep Signed | sed -e 's/.*@/ /g' | sort | uniq
>  citrix.com
>  citrix.com>
>  debian.org>
>  eu.citrix.com>
>  intel.com>
>  jp.fujitsu.com>
>  net-space.pl>
>  novell.com>
>  oracle.com>
>  redhat.com>
>  sun.com>
>  suse.com>
>  us.ibm.com>
>  verge.net.au>
>  xen.org>
>  xensource.com>

That's a useful list.

Thanks,
Ian.

Ian Jackson July 11, 2016, 3:47 p.m. UTC | #18

Ian Jackson writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> Boris Ostrovsky writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
> > But do we need to look at who touched the file or who is explicitly
> > listed as copyright holder (in files' headers)?
> 
> You need to look at the history.
> 
> Sadly, we don't have any means of keeping the copyright headers up to
> date with lists of contributors.

Wait a moment, Lars points out that you are moving only certain bits
of these files into libxl.

Only the authorship of the moved parts is relevant.

Ian.

Boris Ostrovsky July 11, 2016, 4:07 p.m. UTC | #19

On 07/11/2016 11:38 AM, Ian Jackson wrote:
> Boris Ostrovsky writes ("Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code"):
>> But do we need to look at who touched the file or who is explicitly
>> listed as copyright holder (in files' headers)?
> You need to look at the history.
>
> Sadly, we don't have any means of keeping the copyright headers up to
> date with lists of contributors.
>
>> So I did (because I thought only Signed-off might important, is it?)
> No.  In general I think From: is probably more relevant but I would
> include both From: and S-o-b:.
>
> If there are edge cases that crop up only once or twice they could be
> excluded.

OK, then with contribution count:

ostr@workbase> git log . | egrep "Signed|From" | sed -e 's/.*@/ /g' |
sort | uniq -c
      1  citrix.com
     73  citrix.com>
      1  debian.org>
      5  eu.citrix.com>
     18  intel.com>
      2  jp.fujitsu.com>
      1  net-space.pl>
      1  novell.com>
      1  oracle.com>
      1  redhat.com>
      1  sun.com>
      8  suse.com>
      1  us.ibm.com>
      6  verge.net.au>
      4  xen.org>
     25  xensource.com>


debian.com is a single trivial patch:
   
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=83f34fdcdd26c3dcc793c571e7b75c705bd92e7a

fujitsu is one trivial and one somewhat less trivial
   
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=e451db15ef6198f5d21b84618c833ac276087d70
   
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=ab438874b6a8ae955b337c36e7b3204e29b8d407

net-space.pl is Daniel Kiper (who is at Oracle now)

redhat is one simple patch (that has been reverted):
   
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=e4fd0475a08fda414da27c4e57b568f147cfc07e

ibm is one significant patch:
   
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=9fd9787b0e7995ac5f2da504b92723c24d6a3737

verge is a bunch of patches by Simon Horman who probably still is at
that address (I see a message from him on the list from lat December)

xensource and xen.org tags are all from Keir.



So it seems that we need to get permission to convert GPLv2 to LGPL from

Citrix
Intel
Suse/Novell
Oracle/Sun
Simon Horman/Verge
Keir (his commits signed from @xen.org are all from 2011, I don't know
whether that was still while he was at Citrix/Xensource)

+maybe IBM.

-boris

Jan Beulich Aug. 1, 2016, 10:09 a.m. UTC | #20

>>> On 08.07.16 at 18:14, <[e-mail]> wrote:
> On 07/08/2016 11:11 AM, Jan Beulich wrote:
>>>>> On 08.07.16 at 16:39, <[e-mail]> wrote:
>>> On 07/08/2016 06:10 AM, Jan Beulich wrote:
>>>>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, 
>>> unsigned int physical)
>>>>>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>>>>>                   sizeof(struct acpi_20_rsdp));
>>>>>  
>>>>> -    if ( !new_vm_gid(acpi_info) )
>>>>> +    if ( !new_vm_gid(&config->ainfo) )
>>>>>          goto oom;
>>>>>  
>>>>> -    acpi_info->com1_present = uart_exists(0x3f8);
>>>>> -    acpi_info->com2_present = uart_exists(0x2f8);
>>>>> -    acpi_info->lpt1_present = lpt_exists(0x378);
>>>>> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>>>>> -    acpi_info->pci_min = pci_mem_start;
>>>>> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
>>>>> -    if ( pci_hi_mem_end > pci_hi_mem_start )
>>>>> -    {
>>>>> -        acpi_info->pci_hi_min = pci_hi_mem_start;
>>>>> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>>>>> -    }
>>>>> +    *(struct acpi_info *)config->ainfop = config->ainfo;
>>>> With your new separation of responsibilities - does this really
>>>> belong here rather than in the caller? 
>>> I think it should be done here: when the call returns all tables are
>>> already in memory. If the caller wants to load those tables separately
>>> (as probably the toolstack will) then it can simply copy it as a blob.
>> But this structure isn't part of the ACPI tables, and by not doing
>> it here (a) at least some of the intended callers may be able to
>> get away without the ugly cast and (b) the field now named
>> ainfop wouldn't be needed either afaict.
> 
> 
> I probably didn't use right terminology. This is not a table, but an AML
> piece?

Clearly not. This is data structure we define ourselves, which only
gets used by AML code.

> In any case, it's something that is ACPI-specific and I was
> hoping we wouldn't need to expose this to the caller.

That would imo be a relevant argument only if the structure
type was indeed private to a single (sub-)component.

> The fact that it
> is passed in the right format in struct acpi_info is a happy
> coincidence. We may change it in the future (and so perhaps I should
> drop the comment in libacpi.h about "This must match the
> Field("BIOS"....) definition in the DSDT.")

Definitely not: The two absolutely have to remain in sync. They're
C and AML representations of the same thing.

Jan

Boris Ostrovsky Aug. 1, 2016, 2:06 p.m. UTC | #21

On 08/01/2016 06:09 AM, Jan Beulich wrote:
>>>> On 08.07.16 at 18:14, <[e-mail]> wrote:
>> On 07/08/2016 11:11 AM, Jan Beulich wrote:
>>>>>> On 08.07.16 at 16:39, <[e-mail]> wrote:
>>>> On 07/08/2016 06:10 AM, Jan Beulich wrote:
>>>>>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, 
>>>> unsigned int physical)
>>>>>>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>>>>>>                   sizeof(struct acpi_20_rsdp));
>>>>>>  
>>>>>> -    if ( !new_vm_gid(acpi_info) )
>>>>>> +    if ( !new_vm_gid(&config->ainfo) )
>>>>>>          goto oom;
>>>>>>  
>>>>>> -    acpi_info->com1_present = uart_exists(0x3f8);
>>>>>> -    acpi_info->com2_present = uart_exists(0x2f8);
>>>>>> -    acpi_info->lpt1_present = lpt_exists(0x378);
>>>>>> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>>>>>> -    acpi_info->pci_min = pci_mem_start;
>>>>>> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
>>>>>> -    if ( pci_hi_mem_end > pci_hi_mem_start )
>>>>>> -    {
>>>>>> -        acpi_info->pci_hi_min = pci_hi_mem_start;
>>>>>> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>>>>>> -    }
>>>>>> +    *(struct acpi_info *)config->ainfop = config->ainfo;
>>>>> With your new separation of responsibilities - does this really
>>>>> belong here rather than in the caller? 
>>>> I think it should be done here: when the call returns all tables are
>>>> already in memory. If the caller wants to load those tables separately
>>>> (as probably the toolstack will) then it can simply copy it as a blob.
>>> But this structure isn't part of the ACPI tables, and by not doing
>>> it here (a) at least some of the intended callers may be able to
>>> get away without the ugly cast and (b) the field now named
>>> ainfop wouldn't be needed either afaict.
>>
>> I probably didn't use right terminology. This is not a table, but an AML
>> piece?
> Clearly not. This is data structure we define ourselves, which only
> gets used by AML code.
>
>> In any case, it's something that is ACPI-specific and I was
>> hoping we wouldn't need to expose this to the caller.
> That would imo be a relevant argument only if the structure
> type was indeed private to a single (sub-)component.

As I said below, I only expose this structure to callers for convenience.

How about I keep acpi_info private to the builder and instead define new
structure that will pass necessary information to the builder so that it
can fill acpi_info internally and copy it to memory?

This will also have side effect of allowing us to keep acpi_info and
tables in contiguous memory. We do this now implicitly for hvmloader
(RESERVED_MEMORY_DYNAMIC_START = ACPI_INFO_PHYSICAL_ADDRESS + PAGE_SIZE)
so nothing should change from that side but from tools perspective it's
easier to deal with a single ACPI blob.

-boris


>
>> The fact that it
>> is passed in the right format in struct acpi_info is a happy
>> coincidence. We may change it in the future (and so perhaps I should
>> drop the comment in libacpi.h about "This must match the
>> Field("BIOS"....) definition in the DSDT.")
> Definitely not: The two absolutely have to remain in sync. They're
> C and AML representations of the same thing.
>
> Jan
>

Jan Beulich Aug. 1, 2016, 2:18 p.m. UTC | #22

>>> On 01.08.16 at 16:06, <[e-mail]> wrote:
> On 08/01/2016 06:09 AM, Jan Beulich wrote:
>>>>> On 08.07.16 at 18:14, <[e-mail]> wrote:
>>> On 07/08/2016 11:11 AM, Jan Beulich wrote:
>>>>>>> On 08.07.16 at 16:39, <[e-mail]> wrote:
>>>>> On 07/08/2016 06:10 AM, Jan Beulich wrote:
>>>>>>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config, 
>>>>> unsigned int physical)
>>>>>>>                   offsetof(struct acpi_20_rsdp, extended_checksum),
>>>>>>>                   sizeof(struct acpi_20_rsdp));
>>>>>>>  
>>>>>>> -    if ( !new_vm_gid(acpi_info) )
>>>>>>> +    if ( !new_vm_gid(&config->ainfo) )
>>>>>>>          goto oom;
>>>>>>>  
>>>>>>> -    acpi_info->com1_present = uart_exists(0x3f8);
>>>>>>> -    acpi_info->com2_present = uart_exists(0x2f8);
>>>>>>> -    acpi_info->lpt1_present = lpt_exists(0x378);
>>>>>>> -    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>>>>>>> -    acpi_info->pci_min = pci_mem_start;
>>>>>>> -    acpi_info->pci_len = pci_mem_end - pci_mem_start;
>>>>>>> -    if ( pci_hi_mem_end > pci_hi_mem_start )
>>>>>>> -    {
>>>>>>> -        acpi_info->pci_hi_min = pci_hi_mem_start;
>>>>>>> -        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>>>>>>> -    }
>>>>>>> +    *(struct acpi_info *)config->ainfop = config->ainfo;
>>>>>> With your new separation of responsibilities - does this really
>>>>>> belong here rather than in the caller? 
>>>>> I think it should be done here: when the call returns all tables are
>>>>> already in memory. If the caller wants to load those tables separately
>>>>> (as probably the toolstack will) then it can simply copy it as a blob.
>>>> But this structure isn't part of the ACPI tables, and by not doing
>>>> it here (a) at least some of the intended callers may be able to
>>>> get away without the ugly cast and (b) the field now named
>>>> ainfop wouldn't be needed either afaict.
>>>
>>> I probably didn't use right terminology. This is not a table, but an AML
>>> piece?
>> Clearly not. This is data structure we define ourselves, which only
>> gets used by AML code.
>>
>>> In any case, it's something that is ACPI-specific and I was
>>> hoping we wouldn't need to expose this to the caller.
>> That would imo be a relevant argument only if the structure
>> type was indeed private to a single (sub-)component.
> 
> As I said below, I only expose this structure to callers for convenience.
> 
> How about I keep acpi_info private to the builder and instead define new
> structure that will pass necessary information to the builder so that it
> can fill acpi_info internally and copy it to memory?

Well, let's see how that ends up being.

Jan

Patch

diff --git a/tools/firmware/hvmloader/acpi/acpi2_0.h b/tools/firmware/hvmloader/acpi/acpi2_0.h
index 78eb43d..ddcee74 100644
--- a/tools/firmware/hvmloader/acpi/acpi2_0.h
+++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
@@ -449,17 +449,6 @@  struct acpi_20_slit {
 #define ACPI_2_0_SRAT_REVISION 0x01
 #define ACPI_2_0_SLIT_REVISION 0x01
 
-#pragma pack ()
-
-struct acpi_config {
-    unsigned char *dsdt_anycpu;
-    int dsdt_anycpu_len;
-    unsigned char *dsdt_15cpu;
-    int dsdt_15cpu_len;
-};
-
-void acpi_build_tables(struct acpi_config *config, unsigned int physical);
-
 #endif /* _ACPI_2_0_H_ */
 
 /*
diff --git a/tools/firmware/hvmloader/acpi/build.c b/tools/firmware/hvmloader/acpi/build.c
index 1f7103e..1092b4e 100644
--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -16,6 +16,7 @@ 
  */
 
 #include "acpi2_0.h"
+#include "libacpi.h"
 #include "ssdt_s3.h"
 #include "ssdt_s4.h"
 #include "ssdt_tpm.h"
@@ -38,24 +39,6 @@  extern struct acpi_20_fadt Fadt;
 extern struct acpi_20_facs Facs;
 extern struct acpi_20_waet Waet;
 
-/*
- * Located at ACPI_INFO_PHYSICAL_ADDRESS.
- *
- * This must match the Field("BIOS"....) definition in the DSDT.
- */
-struct acpi_info {
-    uint8_t  com1_present:1;    /* 0[0] - System has COM1? */
-    uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
-    uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
-    uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */
-    uint16_t nr_cpus;           /* 2    - Number of CPUs */
-    uint32_t pci_min, pci_len;  /* 4, 8 - PCI I/O hole boundaries */
-    uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
-    uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
-    uint32_t vm_gid_addr;       /* 20   - Address of VM generation id buffer */
-    uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
-};
-
 static void set_checksum(
     void *table, uint32_t checksum_offset, uint32_t length)
 {
@@ -358,7 +341,7 @@  static int construct_secondary_tables(unsigned long *table_ptrs,
     }
 
     /* HPET. */
-    if ( hpet_exists(ACPI_HPET_ADDRESS) )
+    if ( info->hpet_present )
     {
         hpet = construct_hpet();
         if (!hpet) return -1;
@@ -493,9 +476,8 @@  static int new_vm_gid(struct acpi_info *acpi_info)
     return 1;
 }
 
-void acpi_build_tables(struct acpi_config *config, unsigned int physical)
+void acpi_build_tables(struct acpi_config *config)
 {
-    struct acpi_info *acpi_info;
     struct acpi_20_rsdp *rsdp;
     struct acpi_20_rsdt *rsdt;
     struct acpi_20_xsdt *xsdt;
@@ -506,11 +488,6 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
     unsigned long        secondary_tables[ACPI_MAX_SECONDARY_TABLES];
     int                  nr_secondaries, i;
 
-    /* Allocate and initialise the acpi info area. */
-    mem_hole_populate_ram(ACPI_INFO_PHYSICAL_ADDRESS >> PAGE_SHIFT, 1);
-    acpi_info = (struct acpi_info *)ACPI_INFO_PHYSICAL_ADDRESS;
-    memset(acpi_info, 0, sizeof(*acpi_info));
-
     /*
      * Fill in high-memory data structures, starting at @buf.
      */
@@ -570,7 +547,8 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
                  offsetof(struct acpi_header, checksum),
                  sizeof(struct acpi_20_fadt));
 
-    nr_secondaries = construct_secondary_tables(secondary_tables, acpi_info);
+    nr_secondaries = construct_secondary_tables(secondary_tables,
+                                                &config->ainfo);
     if ( nr_secondaries < 0 )
         goto oom;
 
@@ -603,7 +581,7 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
     /*
      * Fill in low-memory data structures: acpi_info and RSDP.
      */
-    rsdp = (struct acpi_20_rsdp *)physical;
+    rsdp = (struct acpi_20_rsdp *)config->rsdp;
 
     memcpy(rsdp, &Rsdp, sizeof(struct acpi_20_rsdp));
     rsdp->rsdt_address = (unsigned long)rsdt;
@@ -615,20 +593,10 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
                  offsetof(struct acpi_20_rsdp, extended_checksum),
                  sizeof(struct acpi_20_rsdp));
 
-    if ( !new_vm_gid(acpi_info) )
+    if ( !new_vm_gid(&config->ainfo) )
         goto oom;
 
-    acpi_info->com1_present = uart_exists(0x3f8);
-    acpi_info->com2_present = uart_exists(0x2f8);
-    acpi_info->lpt1_present = lpt_exists(0x378);
-    acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
-    acpi_info->pci_min = pci_mem_start;
-    acpi_info->pci_len = pci_mem_end - pci_mem_start;
-    if ( pci_hi_mem_end > pci_hi_mem_start )
-    {
-        acpi_info->pci_hi_min = pci_hi_mem_start;
-        acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
-    }
+    *(struct acpi_info *)config->ainfop = config->ainfo;
 
     return;
 
diff --git a/tools/firmware/hvmloader/acpi/libacpi.h b/tools/firmware/hvmloader/acpi/libacpi.h
new file mode 100644
index 0000000..098eee1
--- /dev/null
+++ b/tools/firmware/hvmloader/acpi/libacpi.h
@@ -0,0 +1,80 @@ 
+/******************************************************************************
+ * libacpi.h
+ * 
+ * libacpi interfaces
+ * 
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+
+#ifndef __LIBACPI_H__
+#define __LIBACPI_H__
+
+#pragma pack ()
+
+/*
+ * This must match the Field("BIOS"....) definition in the DSDT.
+ */
+struct acpi_info {
+    uint8_t  com1_present:1;    /* 0[0] - System has COM1? */
+    uint8_t  com2_present:1;    /* 0[1] - System has COM2? */
+    uint8_t  lpt1_present:1;    /* 0[2] - System has LPT1? */
+    uint8_t  hpet_present:1;    /* 0[3] - System has HPET? */
+    uint16_t nr_cpus;           /* 2    - Number of CPUs */
+    uint32_t pci_min, pci_len;  /* 4, 8 - PCI I/O hole boundaries */
+    uint32_t madt_csum_addr;    /* 12   - Address of MADT checksum */
+    uint32_t madt_lapic0_addr;  /* 16   - Address of first MADT LAPIC struct */
+    uint32_t vm_gid_addr;       /* 20   - Address of VM generation id buffer */
+    uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
+};
+
+struct acpi_config {
+    const unsigned char *dsdt_anycpu;
+    unsigned int dsdt_anycpu_len;
+    const unsigned char *dsdt_15cpu;
+    unsigned int dsdt_15cpu_len;
+
+    /* May have some fields updated by acpi_build_table() */
+    struct acpi_info ainfo;
+    /*
+     * Address where acpi_info should be placed.
+     * This must match the OperationRegion(BIOS, SystemMemory, ....)
+     * definition in the DSDT
+     */
+    unsigned int ainfop;
+
+    /* RSDP address */
+    unsigned int rsdp;
+};
+
+void acpi_build_tables(struct acpi_config *config);
+
+#endif /* __LIBACPI_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/firmware/hvmloader/seabios.c b/tools/firmware/hvmloader/seabios.c
index 9f66a79..2fbc3af 100644
--- a/tools/firmware/hvmloader/seabios.c
+++ b/tools/firmware/hvmloader/seabios.c
@@ -26,6 +26,7 @@ 
 
 #include "smbios_types.h"
 #include "acpi/acpi2_0.h"
+#include "acpi/libacpi.h"
 
 #define ROM_INCLUDE_SEABIOS
 #include "roms.inc"
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 9af29b1..4293efa 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -22,6 +22,7 @@ 
 #include "hypercall.h"
 #include "ctype.h"
 #include "acpi/acpi2_0.h"
+#include "acpi/libacpi.h"
 #include <stdint.h>
 #include <xen/xen.h>
 #include <xen/memory.h>
@@ -860,7 +861,25 @@  int hpet_exists(unsigned long hpet_base)
 void hvmloader_acpi_build_tables(struct acpi_config *config,
                                  unsigned int physical)
 {
-    acpi_build_tables(config, physical);
+    /* Allocate and initialise the acpi info area. */
+    mem_hole_populate_ram(ACPI_INFO_PHYSICAL_ADDRESS >> PAGE_SHIFT, 1);
+
+    config->ainfo.com1_present = uart_exists(0x3f8);
+    config->ainfo.com2_present = uart_exists(0x2f8);
+    config->ainfo.lpt1_present = lpt_exists(0x378);
+    config->ainfo.hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
+    config->ainfo.pci_min = pci_mem_start;
+    config->ainfo.pci_len = pci_mem_end - pci_mem_start;
+    if ( pci_hi_mem_end > pci_hi_mem_start )
+    {
+        config->ainfo.pci_hi_min = pci_hi_mem_start;
+        config->ainfo.pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
+    }
+
+    config->rsdp = physical;
+    config->ainfop = ACPI_INFO_PHYSICAL_ADDRESS;
+
+    acpi_build_tables(config);
 }
 
 /*