Re: [Xen-devel] [PATCH v2 03/23] acpi/hvmloader: Initialize vm_gid data outside ACPI code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.16 at 17:09, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 08/09/2016 10:47 AM, Andrew Cooper wrote:
>> On 09/08/16 15:31, Jan Beulich wrote:
>>>>>> On 09.08.16 at 15:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>> On 08/09/2016 09:11 AM, Jan Beulich wrote:
>>>>>>>> On 04.08.16 at 23:06, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>> --- a/tools/firmware/hvmloader/acpi/build.c
>>>>>> +++ b/tools/firmware/hvmloader/acpi/build.c
>>>>>> @@ -462,32 +462,26 @@ static int construct_secondary_tables(unsigned 
>>>>>> long 
> *table_ptrs,
>>>>>>   *
>>>>>>   * Return 0 if memory failure, != 0 if success
>>>>>>   */
>>>>>> -static int new_vm_gid(struct acpi_info *acpi_info)
>>>>>> +static int new_vm_gid(struct acpi_config *config,
>>>>>> +                      struct acpi_info *info)
>>>>>>  {
>>>>>> -    uint64_t vm_gid[2], *buf;
>>>>>> -    const char * s;
>>>>>> -    char *end;
>>>>>> -
>>>>>> -    acpi_info->vm_gid_addr = 0;
>>>>>> -
>>>>>> -    /* read ID and check for 0 */
>>>>>> -    s = xenstore_read("platform/generation-id", "0:0");
>>>>>> -    vm_gid[0] = strtoll(s, &end, 0);
>>>>>> -    vm_gid[1] = 0;
>>>>>> -    if ( end && end[0] == ':' )
>>>>>> -        vm_gid[1] = strtoll(end+1, NULL, 0);
>>>>>> -    if ( !vm_gid[0] && !vm_gid[1] )
>>>>>> +    uint64_t *buf;
>>>>>> +
>>>>>> +    info->vm_gid_addr = 0;
>>>>>> +
>>>>>> +    /* check for 0 ID*/
>>>>>> +    if ( !config->vm_gid[0] && !config->vm_gid[1] )
>>>>>>          return 1;
>>>>>>  
>>>>>>      /* copy to allocate BIOS memory */
>>>>>> -    buf = (uint64_t *) mem_alloc(sizeof(vm_gid), 8);
>>>>>> +    buf = mem_alloc(sizeof(config->vm_gid), 8);
>>>>>>      if ( !buf )
>>>>>>          return 0;
>>>>>> -    memcpy(buf, vm_gid, sizeof(vm_gid));
>>>>>> +    memcpy(buf, config->vm_gid, sizeof(config->vm_gid));
>>>>>>  
>>>>>> -    /* set into ACPI table and HVM param the address */
>>>>>> -    acpi_info->vm_gid_addr = virt_to_phys(buf);
>>>>>> -    hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, 
>>>>>> acpi_info->vm_gid_addr);
>>>>>> +    /* set the address into ACPI table and also pass it back to the 
>>>>>> caller */
>>>>>> +    info->vm_gid_addr = virt_to_phys(buf);
>>>>> What consumer of this value is left? Can't that field go away now?
>>>> In acpi_info? TBH, I don't know, I thought this was introduced to deal
>>>> with something in Windows (commit 978cc62b looks generic but then
>>>> c5a29a87 is for Windows).
>>> Sure, but the need for the field appears to go away with the
>>> introduction of the one in acpi_config.
>> The way genid needs to work is this:
>>
>> 1) Allocate some memory, reserving it in the e820
>> 2) Represents that physical address in the ACPI tables for the guest to
>> consume
>> 3) Update Xenstore with the physical address, for the toolstack to consume
>>
>> So long as HVMLoader continues to function in that way, it is fine.
> 
> What about VGID device in dsdt.asl? Doesn't it want to see the address
> stored in VGIA, which is acpi_info->vm_gid_addr?

Oh, right, of course. So please simply eliminate the duplicate
virt_to_phys() invocation, slightly decreasing the size of one
of the later patches at once.

Jan


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