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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/01/2016 06:09 AM, Jan Beulich wrote:
>>>> 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.

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
>



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