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

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: wei.liu2@citrix.com, andrew.cooper3@citrix.com,
	ian.jackson@eu.citrix.com, xen-devel@lists.xen.org,
	julien.grall@arm.com, zhaoshenglong@huawei.com,
	roger.pau@citrix.com
Subject: Re: [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code
Date: Fri, 8 Jul 2016 12:14:29 -0400	[thread overview]
Message-ID: <59a56e5d-33f5-6cd6-08c2-e48a23c39c56@oracle.com> (raw)
In-Reply-To: <577FDED202000078000FCC01@prv-mh.provo.novell.com>

On 07/08/2016 11:11 AM, Jan Beulich wrote:
>>>> On 08.07.16 at 16:39, <boris.ostrovsky@oracle.com> 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

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