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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.08.16 at 16:06, <boris.ostrovsky@xxxxxxxxxx> wrote:
> 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?

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

Jan


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