Re: [Xen-devel] [PATCH RFC 18/20] libxc/acpi: Build ACPI tables for HVMlite guests

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/16/2016 04:54 AM, Wei Liu wrote:
> On Mon, Jun 06, 2016 at 11:15:22AM -0400, Boris Ostrovsky wrote:
> [...]
>>> +static int init_acpi_config(struct xc_dom_image *dom,
>>> +                            struct acpi_config *config)
>>> +{
>>> +    xc_interface *xch = dom->xch;
>>> +    uint32_t domid = dom->guest_domid;
>>> +    xc_dominfo_t info;
>>> +    int i, rc;
>>> +
>>> +    memset(config, 0, sizeof(*config));
>>> +
>>> +    config->dsdt_anycpu = config->dsdt_15cpu = dsdt_empty;
>>> +    config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_empty_len;
>>> +
>>> +    rc = xc_domain_getinfo(xch, domid, 1, &info);
>>> +    if ( rc < 0 )
>>> +    {
>>> +        DOMPRINTF("%s: getdomaininfo failed (rc=%d)", __FUNCTION__, rc);
>>> +        return rc;
>>> +    }
>>> +
>>> +    config->apic_mode = 1;
>>> +
>>> +    if ( dom->nr_vnodes )
>>> +    {
>>> +        struct acpi_numa *numa = &config->numa;
>>> +
>>> +        numa->vmemrange = calloc(dom->nr_vmemranges,
>>> +                                 sizeof(*numa->vmemrange));
>>> +        numa->vdistance = calloc(dom->nr_vnodes,
>>> +                                 sizeof(*numa->vdistance));
>>> +        numa->vcpu_to_vnode = calloc(config->nr_vcpus,
>>> +                                     sizeof(*numa->vcpu_to_vnode));
>>> +        if ( !numa->vmemrange || !numa->vdistance || !numa->vcpu_to_vnode )
>>> +        {
>>> +            DOMPRINTF("%s: Out of memory", __FUNCTION__);
>>> +            free(numa->vmemrange);
>>> +            free(numa->vdistance);
>>> +            free(numa->vcpu_to_vnode);
>>> +            return -ENOMEM;
>>> +        }
>>> +
>>> +        rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes,
>>> +                                &numa->nr_vmemranges,
>>> +                                &config->nr_vcpus, numa->vmemrange,
>>> +                                numa->vdistance, numa->vcpu_to_vnode);
>>> +
>>> +       if ( rc )
>>> +        {
>>> +            DOMPRINTF("%s: xc_domain_getvnuma failed (rc=%d)", 
>>> __FUNCTION__, rc);
>>> +            return rc;
>>> +        }
>>> +    }
>>> +    else
>>> +        config->nr_vcpus = info.max_vcpu_id + 1;
>>> This looks wrong, at least it is not immediately clear why you would
>>> want to do this.
>>
>> Why is this wrong? If we have one VCPU max_vcpu_id will be zero, won't it?
>>
> I'm not saying it is absolutely wrong, just that I don't quite
> understand why it is coded this way.
>
> It's guarded by dom->nr_vnodes and the code is not immediately clear why
> you want to do that. Perhaps you can add a comment here?

So this actually *is* wrong: I am trying to calloc(config->nr_vcpus,...)
before I call xc_domain_getvnuma(), which sets config->nr_vcpus. And I
suspect the same is true for nr_vmemranges.

I think I lost an earlier call to xc_domain_getvnuma(.., NULL, NULL, NULL).

-boris


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