Re: [RFC XEN PATCH 08/16] tools/libacpi: expose details of memory allocation callback

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 01/27/17 15:58 -0500, Konrad Rzeszutek Wilk wrote:
>On Mon, Oct 10, 2016 at 08:32:27AM +0800, Haozhong Zhang wrote:
>> Expose the minimal allocation unit and the minimal alignment used by the
>> memory allocator, so that certain ACPI code (e.g. the AML builder added
>> later) can get contiguous memory allocated by multiple calls to
>
>s/later/in patch titled: "XYZ"/
>

will do

>> acpi_ctxt.mem_ops.alloc().
>
>This contingous memory is virtual or physical? You may want to be
>specific.
>

physical

>And you may want to say that acpi_build_tables uses that by default
>which is why you have the value of sixteen.

yes

>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> ---
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/firmware/hvmloader/util.c | 2 ++
>>  tools/libacpi/libacpi.h         | 3 +++
>>  tools/libxl/libxl_x86_acpi.c    | 2 ++
>>  3 files changed, 7 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
>> index 1fe8dcc..504ae6a 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -972,6 +972,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>>      ctxt.mem_ops.free = acpi_mem_free;
>>      ctxt.mem_ops.v2p = acpi_v2p;
>>      ctxt.mem_ops.p2v = acpi_p2v;
>> +    ctxt.min_alloc_unit = PAGE_SIZE;
>
>?? Really? That seems excessive as the acpi_build_tables calls
>ctxt->mem_ops.alloc :
>$ grep "ctxt->mem_ops.alloc" * | wc -l
>20
>
>That would imply 20 pages ?
>

I'm wrong here. I just relooked at mem_ops.alloc provided by hvmloader
and libxl_x86_acpi and found both allocates by byte, rather than by page.
I'll remove this field.

>> +    ctxt.min_alloc_align = 16;
>
>Does that mean it is sixteen pages aligment? Or 16 bytes aligment?
>
>If the bytes perhaps you want to change the name to
>'min_alloc_byte_align' ?

It's for byte alignment. I'll rename this field.

Thanks,
Haozhong

>
>>
>>      acpi_build_tables(&ctxt, config);
>>
>> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
>> index 62e90ab..0fb16e7 100644
>> --- a/tools/libacpi/libacpi.h
>> +++ b/tools/libacpi/libacpi.h
>> @@ -47,6 +47,9 @@ struct acpi_ctxt {
>>          unsigned long (*v2p)(struct acpi_ctxt *ctxt, void *v);
>>          void *(*p2v)(struct acpi_ctxt *ctxt, unsigned long p);
>>      } mem_ops;
>> +
>> +    uint32_t min_alloc_unit;
>> +    uint32_t min_alloc_align;
>>  };
>>
>>  struct acpi_config {
>> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
>> index aa5b83d..baf60ac 100644
>> --- a/tools/libxl/libxl_x86_acpi.c
>> +++ b/tools/libxl/libxl_x86_acpi.c
>> @@ -187,6 +187,8 @@ int libxl__dom_load_acpi(libxl__gc *gc,
>>      libxl_ctxt.c.mem_ops.v2p = virt_to_phys;
>>      libxl_ctxt.c.mem_ops.p2v = phys_to_virt;
>>      libxl_ctxt.c.mem_ops.free = acpi_mem_free;
>> +    libxl_ctxt.c.min_alloc_unit = libxl_ctxt.page_size;
>> +    libxl_ctxt.c.min_alloc_align = 16;
>>
>>      rc = init_acpi_config(gc, dom, b_info, &config);
>>      if (rc) {
>> --
>> 2.10.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel

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