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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.07.16 at 18:14, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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?

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


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