Re: [PATCH RFC 03/20] acpi/hvmloader: Initialize vm_gid data outside ACPI code

["Jan Beulich" <JBeulich@suse.com>]

>>> On 06.04.16 at 03:25, <boris.ostrovsky@oracle.com> wrote:
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -448,32 +448,24 @@ 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)
>  {
> -    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;
> +
> +    config->acpi_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 = (uint64_t *) 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);
> +    config->acpi_info.vm_gid_addr = virt_to_phys(buf);

Here things get interesting: If different entities are responsible for
filling different parts of acpi_info, I think this should be documented
in the structure definition. Can't this be left to the entity initializing
that structure?

> @@ -875,7 +878,19 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>          config->acpi_info.pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>      }
>  
> +    s = xenstore_read("platform/generation-id", "0:0");
> +    if (s) {

Please use hypervisor coding style here; Interestingly you do so ...

> +        char *end;
> +
> +        config->vm_gid[0] = strtoll(s, &end, 0);
> +        config->vm_gid[1] = 0;
> +        if ( end && end[0] == ':' )

... here.

Jan


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