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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 07/08/2016 11:11 AM, Jan Beulich wrote:
>>>> On 08.07.16 at 16:39, <boris.ostrovsky@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel