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 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.

>
>> +struct acpi_config {
>> +    const unsigned char *dsdt_anycpu;
>> +    unsigned int dsdt_anycpu_len;
>> +    const unsigned char *dsdt_15cpu;
>> +    unsigned int dsdt_15cpu_len;
>> +
>> +    /* May have some fields updated by acpi_build_table() */
>> +    struct acpi_info ainfo;
>> +    /*
>> +     * Address where acpi_info should be placed.
>> +     * This must match the OperationRegion(BIOS, SystemMemory, ....)
>> +     * definition in the DSDT
>> +     */
>> +    unsigned int ainfop;
> What is the "a" prefix of these two fields good for? Considering the
> name of the structure I don't see a need for any prefix. But if you
> absolutely want one, then acpi_ please.

You requested 'acpi_' prefix to be dropped earlier. I added 'a' (and
'pt_' in patch 5) to be a bit more cscope-friendly: names like info,
addr and length will result in lots of unnecessary hits.

-boris




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