Re: [Xen-devel] [PATCH v1 10/20] acpi/hvmloader: Replace mem_alloc() and virt_to_phys() with memory ops

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.07.16 at 21:05, <boris.ostrovsky@xxxxxxxxxx> wrote:
> Components that wish to use ACPI builder will need to provide their own
> mem_alloc() and virt_to_phys() routines. Pointers to these routines will
> be passed to the builder as memory ops.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> ---
> 
> Changes in v1:
> * Keep memory ops seprate from acpi_config, in struct acpi_context.
> 
> Jan requested adding a free() op to struct acpi_mem_ops. I couldn't see who 
> might want to
> use those. The builder uses (should use) mem_alloc() to allocate memory for 
> tables, not as a
> general-purpose allocator.

In addition to what I said back then, did you think of error cleanup
paths here? Not all errors mean the guest has to die.

> @@ -453,18 +462,18 @@ static int new_vm_gid(struct acpi_config *config)
>          return 1;
>  
>      /* copy to allocate BIOS memory */
> -    buf = (uint64_t *) mem_alloc(sizeof(config->vm_gid), 8);
> +    buf = (uint64_t *) ctxt->mem_ops.alloc(ctxt, sizeof(config->vm_gid), 8);

Once again this appears to be a cast that already before was
unnecessary. So it (and in any event the stray blank) should
perhaps already get deleted when you touch this line the first
time (in one of the earlier patches).

> --- a/tools/firmware/hvmloader/acpi/libacpi.h
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -64,6 +64,13 @@ struct acpi_numa {
>      xen_vmemrange_t *vmemrange;
>  };
>  
> +struct acpi_ctxt {
> +    struct acpi_mem_ops {
> +        void *(*alloc)(struct acpi_ctxt *ctxt, uint32_t size, uint32_t 
> align);
> +        unsigned long (*v2p)(struct acpi_ctxt *ctxt, void *v);
> +    } mem_ops;
> +};
> +
>  struct acpi_config {
>      const unsigned char *dsdt_anycpu;
>      unsigned int dsdt_anycpu_len;

While you mention this in the revision log, I don't see the reason
for keeping this fully separate. Quite a few of the changes you
do here could be avoided if the new structure got pointed to by a
field in struct acpi_config.

Jan


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