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

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

I remember having that discussion once with Mukesh too. I continue
to think source code should not get uglified just because of some
random tool's limitations, maybe unless _everyone_ uses that tool.

Jan


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