diff mbox

[RFC,14/20] acpi/hvmloader: Replace mem_alloc() and virt_to_phys() with memory ops

Message ID 1459905949-10358-15-git-send-email-boris.ostrovsky@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris Ostrovsky April 6, 2016, 1:25 a.m. UTC 
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@oracle.com>
---
 tools/firmware/hvmloader/acpi/acpi2_0.h |   6 ++
 tools/firmware/hvmloader/acpi/build.c   | 104 +++++++++++++++++---------------
 tools/firmware/hvmloader/util.c         |   8 +++
 tools/firmware/hvmloader/util.h         |   2 +-
 4 files changed, 70 insertions(+), 50 deletions(-)

Comments

Jan Beulich June 6, 2016, 12:58 p.m. UTC | #1 
>>> On 06.04.16 at 03:25, <boris.ostrovsky@oracle.com> wrote:
> --- a/tools/firmware/hvmloader/acpi/acpi2_0.h
> +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
> @@ -490,6 +490,11 @@ struct acpi_numa {
>      xen_vmemrange_t *vmemrange;
>  };
>  
> +struct acpi_mem_ops {
> +    void *(*alloc)(uint32_t size, uint32_t align);
> +    unsigned long (*v2p)(void *v);
> +};

As before, I think this is the wrong header for such stuff.

And then I think I agree with the allocation hook (or otherwise the
too generic name would need to be changed), but I don't think I
agree with the v2p one. Instead I think the consumer should
provide a suitable virt_to_phys(): That's already the case for
hvmloader, should be trivial for libxc. And as long as we provide
Dom0 with a 1:1 machine/physical addresses, the existing one in
the hypervisor would do too.

At the very least alternatively, to reduce code churn, patch order
should be changed, so that you don't touch again all the lines that
you made use virt_to_phys() just a patch or two ago.

Plus I think the allocation hook should be accompanied by a
freeing one, even if for now this may not be used. Iirc there are
cases where space gets allocated without being retained, not
getting freed just because the simplistic allocator can't handle
such.

Jan
Boris Ostrovsky June 6, 2016, 3:46 p.m. UTC | #2 
On 06/06/2016 08:58 AM, Jan Beulich wrote:
>>>> On 06.04.16 at 03:25, <boris.ostrovsky@oracle.com> wrote:
>> --- a/tools/firmware/hvmloader/acpi/acpi2_0.h
>> +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
>> @@ -490,6 +490,11 @@ struct acpi_numa {
>>      xen_vmemrange_t *vmemrange;
>>  };
>>  
>> +struct acpi_mem_ops {
>> +    void *(*alloc)(uint32_t size, uint32_t align);
>> +    unsigned long (*v2p)(void *v);
>> +};
> As before, I think this is the wrong header for such stuff.
>
> And then I think I agree with the allocation hook (or otherwise the
> too generic name would need to be changed), but I don't think I
> agree with the v2p one. Instead I think the consumer should
> provide a suitable virt_to_phys(): That's already the case for
> hvmloader, should be trivial for libxc. And as long as we provide
> Dom0 with a 1:1 machine/physical addresses, the existing one in
> the hypervisor would do too.
>
> At the very least alternatively, to reduce code churn, patch order
> should be changed, so that you don't touch again all the lines that
> you made use virt_to_phys() just a patch or two ago.

I'd rather go this route. IIRC I initially had both mem_alloc() and
virt_to_phys() being provided by callers by name but it ended up being
somewhat messy. And I think for consistency sake they should both be
either hooks or provided directly.

-boris
diff mbox

Patch

diff --git a/tools/firmware/hvmloader/acpi/acpi2_0.h b/tools/firmware/hvmloader/acpi/acpi2_0.h
index 0bfd3ed..0525a1e 100644
--- a/tools/firmware/hvmloader/acpi/acpi2_0.h
+++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
@@ -490,6 +490,11 @@  struct acpi_numa {
     xen_vmemrange_t *vmemrange;
 };
 
+struct acpi_mem_ops {
+    void *(*alloc)(uint32_t size, uint32_t align);
+    unsigned long (*v2p)(void *v);
+};
+
 struct acpi_config {
     unsigned char *dsdt_anycpu;
     int dsdt_anycpu_len;
@@ -508,6 +513,7 @@  struct acpi_config {
     struct acpi_numa numa;
     uint16_t *tis_hdr;
     void *acpi_info_page;
+    struct acpi_mem_ops mem_ops;
 };
 
 void acpi_build_tables(struct acpi_config *config, unsigned int physical);
diff --git a/tools/firmware/hvmloader/acpi/build.c b/tools/firmware/hvmloader/acpi/build.c
index 09c1ed3..eb8c632 100644
--- a/tools/firmware/hvmloader/acpi/build.c
+++ b/tools/firmware/hvmloader/acpi/build.c
@@ -82,7 +82,7 @@  static struct acpi_20_madt *construct_madt(struct acpi_config *config)
     sz += sizeof(struct acpi_20_madt_ioapic);
     sz += sizeof(struct acpi_20_madt_lapic) * nr_processor_objects;
 
-    madt = mem_alloc(sz, 16);
+    madt = config->mem_ops.alloc(sz, 16);
     if (!madt) return NULL;
 
     memset(madt, 0, sizeof(*madt));
@@ -139,7 +139,7 @@  static struct acpi_20_madt *construct_madt(struct acpi_config *config)
     else
         lapic = (struct acpi_20_madt_lapic *)(madt + 1);
 
-    config->acpi_info.madt_lapic0_addr = virt_to_phys(lapic);
+    config->acpi_info.madt_lapic0_addr = config->mem_ops.v2p(lapic);
     for ( i = 0; i < nr_processor_objects; i++ )
     {
         memset(lapic, 0, sizeof(*lapic));
@@ -158,16 +158,16 @@  static struct acpi_20_madt *construct_madt(struct acpi_config *config)
     set_checksum(madt, offsetof(struct acpi_header, checksum),
                  madt->header.length);
     config->acpi_info.madt_csum_addr =
-        virt_to_phys(&madt->header.checksum);
+        config->mem_ops.v2p(&madt->header.checksum);
 
     return madt;
 }
 
-static struct acpi_20_hpet *construct_hpet(void)
+static struct acpi_20_hpet *construct_hpet(struct acpi_config *config)
 {
     struct acpi_20_hpet *hpet;
 
-    hpet = mem_alloc(sizeof(*hpet), 16);
+    hpet = config->mem_ops.alloc(sizeof(*hpet), 16);
     if (!hpet) return NULL;
 
     memset(hpet, 0, sizeof(*hpet));
@@ -186,11 +186,11 @@  static struct acpi_20_hpet *construct_hpet(void)
     return hpet;
 }
 
-static struct acpi_20_waet *construct_waet(void)
+static struct acpi_20_waet *construct_waet(struct acpi_config *config)
 {
     struct acpi_20_waet *waet;
 
-    waet = mem_alloc(sizeof(*waet), 16);
+    waet = config->mem_ops.alloc(sizeof(*waet), 16);
     if (!waet) return NULL;
 
     memcpy(waet, &Waet, sizeof(*waet));
@@ -213,7 +213,7 @@  static struct acpi_20_srat *construct_srat(struct acpi_config *config)
     size = sizeof(*srat) + sizeof(*processor) * config->nr_vcpus +
            sizeof(*memory) * config->numa.nr_vmemranges;
 
-    p = mem_alloc(size, 16);
+    p = config->mem_ops.alloc(size, 16);
     if ( !p )
         return NULL;
 
@@ -265,7 +265,7 @@  static struct acpi_20_slit *construct_slit(struct acpi_config *config)
     num = config->numa.nr_vnodes * config->numa.nr_vnodes;
     size = sizeof(*slit) + num * sizeof(uint8_t);
 
-    slit = mem_alloc(size, 16);
+    slit = config->mem_ops.alloc(size, 16);
     if ( !slit )
         return NULL;
 
@@ -314,12 +314,12 @@  static int construct_passthrough_tables(unsigned long *table_ptrs,
 
         header = (struct acpi_header*)acpi_pt_addr;
 
-        buffer = mem_alloc(header->length, 16);
+        buffer = config->mem_ops.alloc(header->length, 16);
         if ( buffer == NULL )
             break;
         memcpy(buffer, header, header->length);
 
-        table_ptrs[nr_tables++] = virt_to_phys(buffer);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(buffer);
         total += header->length;
         acpi_pt_addr += header->length;
     }
@@ -344,49 +344,49 @@  static int construct_secondary_tables(unsigned long *table_ptrs,
     {
         madt = construct_madt(config);
         if (!madt) return -1;
-        table_ptrs[nr_tables++] = virt_to_phys(madt);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(madt);
     }
 
     /* HPET. */
     if ( config->acpi_info.hpet_present )
     {
-        hpet = construct_hpet();
+        hpet = construct_hpet(config);
         if (!hpet) return -1;
-        table_ptrs[nr_tables++] = virt_to_phys(hpet);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(hpet);
     }
 
     /* WAET. */
     if ( config->table_flags & ACPI_BUILD_WAET )
     {
-        waet = construct_waet();
+        waet = construct_waet(config);
         if (!waet) return -1;
-        table_ptrs[nr_tables++] = virt_to_phys(waet);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(waet);
     }
 
     if ( config->table_flags & ACPI_BUILD_SSDT_PM )
     {
-        ssdt = mem_alloc(sizeof(ssdt_pm), 16);
+        ssdt = config->mem_ops.alloc(sizeof(ssdt_pm), 16);
         if (!ssdt) return -1;
         memcpy(ssdt, ssdt_pm, sizeof(ssdt_pm));
-        table_ptrs[nr_tables++] = virt_to_phys(ssdt);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(ssdt);
     }
 
     if ( config->table_flags & ACPI_BUILD_SSDT_S3 )
     {
-        ssdt = mem_alloc(sizeof(ssdt_s3), 16);
+        ssdt = config->mem_ops.alloc(sizeof(ssdt_s3), 16);
         if (!ssdt) return -1;
         memcpy(ssdt, ssdt_s3, sizeof(ssdt_s3));
-        table_ptrs[nr_tables++] = virt_to_phys(ssdt);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(ssdt);
     } else {
         printf("S3 disabled\n");
     }
 
     if ( config->table_flags & ACPI_BUILD_SSDT_S4 )
     {
-        ssdt = mem_alloc(sizeof(ssdt_s4), 16);
+        ssdt = config->mem_ops.alloc(sizeof(ssdt_s4), 16);
         if (!ssdt) return -1;
         memcpy(ssdt, ssdt_s4, sizeof(ssdt_s4));
-        table_ptrs[nr_tables++] = virt_to_phys(ssdt);
+        table_ptrs[nr_tables++] = config->mem_ops.v2p(ssdt);
     } else {
         printf("S4 disabled\n");
     }
@@ -398,15 +398,15 @@  static int construct_secondary_tables(unsigned long *table_ptrs,
              (config->tis_hdr[1] == tis_signature[1]) &&
              (config->tis_hdr[2] == tis_signature[2]) )
         {
-            ssdt = mem_alloc(sizeof(ssdt_tpm), 16);
+            ssdt = config->mem_ops.alloc(sizeof(ssdt_tpm), 16);
             if (!ssdt) return -1;
             memcpy(ssdt, ssdt_tpm, sizeof(ssdt_tpm));
-            table_ptrs[nr_tables++] = virt_to_phys(ssdt);
+            table_ptrs[nr_tables++] = config->mem_ops.v2p(ssdt);
 
-            tcpa = mem_alloc(sizeof(struct acpi_20_tcpa), 16);
+            tcpa = config->mem_ops.alloc(sizeof(struct acpi_20_tcpa), 16);
             if (!tcpa) return -1;
             memset(tcpa, 0, sizeof(*tcpa));
-            table_ptrs[nr_tables++] = virt_to_phys(tcpa);
+            table_ptrs[nr_tables++] = config->mem_ops.v2p(tcpa);
 
             tcpa->header.signature = ACPI_2_0_TCPA_SIGNATURE;
             tcpa->header.length    = sizeof(*tcpa);
@@ -416,9 +416,9 @@  static int construct_secondary_tables(unsigned long *table_ptrs,
             tcpa->header.oem_revision = ACPI_OEM_REVISION;
             tcpa->header.creator_id   = ACPI_CREATOR_ID;
             tcpa->header.creator_revision = ACPI_CREATOR_REVISION;
-            if ( (lasa = mem_alloc(ACPI_2_0_TCPA_LAML_SIZE, 16)) != NULL )
+            if ( (lasa = config->mem_ops.alloc(ACPI_2_0_TCPA_LAML_SIZE, 16)) != NULL )
             {
-                tcpa->lasa = virt_to_phys(lasa);
+                tcpa->lasa = config->mem_ops.v2p(lasa);
                 tcpa->laml = ACPI_2_0_TCPA_LAML_SIZE;
                 memset(lasa, 0, tcpa->laml);
                 set_checksum(tcpa,
@@ -435,11 +435,11 @@  static int construct_secondary_tables(unsigned long *table_ptrs,
         struct acpi_20_slit *slit = construct_slit(config);
 
         if ( srat )
-            table_ptrs[nr_tables++] = virt_to_phys(srat);
+            table_ptrs[nr_tables++] = config->mem_ops.v2p(srat);
         else
             printf("Failed to build SRAT, skipping...\n");
         if ( slit )
-            table_ptrs[nr_tables++] = virt_to_phys(slit);
+            table_ptrs[nr_tables++] = config->mem_ops.v2p(slit);
         else
             printf("Failed to build SLIT, skipping...\n");
     }
@@ -469,13 +469,13 @@  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 *) config->mem_ops.alloc(sizeof(config->vm_gid), 8);
     if ( !buf )
         return 0;
     memcpy(buf, config->vm_gid, sizeof(config->vm_gid));
 
     /* set into ACPI table and HVM param the address */
-    config->acpi_info.vm_gid_addr = virt_to_phys(buf);
+    config->acpi_info.vm_gid_addr = config->mem_ops.v2p(buf);
 
     return 1;
 }
@@ -492,11 +492,17 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
     unsigned long        secondary_tables[ACPI_MAX_SECONDARY_TABLES];
     int                  nr_secondaries, i;
 
+    if ( !config->mem_ops.alloc || !config->mem_ops.v2p )
+    {
+        printf("unable to build ACPI tables: no memory ops\n");
+        return;
+    }
+
     /*
      * Fill in high-memory data structures, starting at @buf.
      */
 
-    facs = mem_alloc(sizeof(struct acpi_20_facs), 16);
+    facs = config->mem_ops.alloc(sizeof(struct acpi_20_facs), 16);
     if (!facs) goto oom;
     memcpy(facs, &Facs, sizeof(struct acpi_20_facs));
 
@@ -510,14 +516,14 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
      */
     if ( config->nr_vcpus <= 15 && config->dsdt_15cpu)
     {
-        dsdt = mem_alloc(config->dsdt_15cpu_len, 16);
+        dsdt = config->mem_ops.alloc(config->dsdt_15cpu_len, 16);
         if (!dsdt) goto oom;
         memcpy(dsdt, config->dsdt_15cpu, config->dsdt_15cpu_len);
         nr_processor_objects = 15;
     }
     else
     {
-        dsdt = mem_alloc(config->dsdt_anycpu_len, 16);
+        dsdt = config->mem_ops.alloc(config->dsdt_anycpu_len, 16);
         if (!dsdt) goto oom;
         memcpy(dsdt, config->dsdt_anycpu, config->dsdt_anycpu_len);
         nr_processor_objects = HVM_MAX_VCPUS;
@@ -531,24 +537,24 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
      * compatible revision 1 FADT that is linked with the RSDT. Refer to:
      *     http://www.acpi.info/presentations/S01USMOBS169_OS%20new.ppt
      */
-    fadt_10 = mem_alloc(sizeof(struct acpi_10_fadt), 16);
+    fadt_10 = config->mem_ops.alloc(sizeof(struct acpi_10_fadt), 16);
     if (!fadt_10) goto oom;
     memcpy(fadt_10, &Fadt, sizeof(struct acpi_10_fadt));
     fadt_10->header.length = sizeof(struct acpi_10_fadt);
     fadt_10->header.revision = ACPI_1_0_FADT_REVISION;
-    fadt_10->dsdt          = virt_to_phys(dsdt);
-    fadt_10->firmware_ctrl = virt_to_phys(facs);
+    fadt_10->dsdt          = config->mem_ops.v2p(dsdt);
+    fadt_10->firmware_ctrl = config->mem_ops.v2p(facs);
     set_checksum(fadt_10,
                  offsetof(struct acpi_header, checksum),
                  sizeof(struct acpi_10_fadt));
 
-    fadt = mem_alloc(sizeof(struct acpi_20_fadt), 16);
+    fadt = config->mem_ops.alloc(sizeof(struct acpi_20_fadt), 16);
     if (!fadt) goto oom;
     memcpy(fadt, &Fadt, sizeof(struct acpi_20_fadt));
-    fadt->dsdt   = virt_to_phys(dsdt);
-    fadt->x_dsdt = virt_to_phys(dsdt);
-    fadt->firmware_ctrl   = virt_to_phys(facs);
-    fadt->x_firmware_ctrl = virt_to_phys(facs);
+    fadt->dsdt   = config->mem_ops.v2p(dsdt);
+    fadt->x_dsdt = config->mem_ops.v2p(dsdt);
+    fadt->firmware_ctrl   = config->mem_ops.v2p(facs);
+    fadt->x_firmware_ctrl = config->mem_ops.v2p(facs);
     set_checksum(fadt,
                  offsetof(struct acpi_header, checksum),
                  sizeof(struct acpi_20_fadt));
@@ -557,12 +563,12 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
     if ( nr_secondaries < 0 )
         goto oom;
 
-    xsdt = mem_alloc(sizeof(struct acpi_20_xsdt)+
+    xsdt = config->mem_ops.alloc(sizeof(struct acpi_20_xsdt)+
                      sizeof(uint64_t)*nr_secondaries,
                      16);
     if (!xsdt) goto oom;
     memcpy(xsdt, &Xsdt, sizeof(struct acpi_header));
-    xsdt->entry[0] = virt_to_phys(fadt);
+    xsdt->entry[0] = config->mem_ops.v2p(fadt);
     for ( i = 0; secondary_tables[i]; i++ )
         xsdt->entry[i+1] = secondary_tables[i];
     xsdt->header.length = sizeof(struct acpi_header) + (i+1)*sizeof(uint64_t);
@@ -570,12 +576,12 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
                  offsetof(struct acpi_header, checksum),
                  xsdt->header.length);
 
-    rsdt = mem_alloc(sizeof(struct acpi_20_rsdt)+
+    rsdt = config->mem_ops.alloc(sizeof(struct acpi_20_rsdt)+
                      sizeof(uint32_t)*nr_secondaries,
                      16);
     if (!rsdt) goto oom;
     memcpy(rsdt, &Rsdt, sizeof(struct acpi_header));
-    rsdt->entry[0] = virt_to_phys(fadt_10);
+    rsdt->entry[0] = config->mem_ops.v2p(fadt_10);
     for ( i = 0; secondary_tables[i]; i++ )
         rsdt->entry[i+1] = secondary_tables[i];
     rsdt->header.length = sizeof(struct acpi_header) + (i+1)*sizeof(uint32_t);
@@ -589,8 +595,8 @@  void acpi_build_tables(struct acpi_config *config, unsigned int physical)
     rsdp = (struct acpi_20_rsdp *)physical;
 
     memcpy(rsdp, &Rsdp, sizeof(struct acpi_20_rsdp));
-    rsdp->rsdt_address = virt_to_phys(rsdt);
-    rsdp->xsdt_address = virt_to_phys(xsdt);
+    rsdp->rsdt_address = config->mem_ops.v2p(rsdt);
+    rsdp->xsdt_address = config->mem_ops.v2p(xsdt);
     set_checksum(rsdp,
                  offsetof(struct acpi_10_rsdp, checksum),
                  sizeof(struct acpi_10_rsdp));
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index c111f61..9e6ee7d 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -866,6 +866,11 @@  static uint8_t battery_port_exists(void)
     return (inb(0x88) == 0x1F);
 }
 
+unsigned long virt_to_phys(void *v)
+{
+    return (unsigned long)v;
+}
+
 void hvmloader_acpi_build_tables(struct acpi_config *config,
                                  unsigned int physical)
 {
@@ -928,6 +933,9 @@  void hvmloader_acpi_build_tables(struct acpi_config *config,
     config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
     config->acpi_info_page = (void *)ACPI_INFO_PHYSICAL_ADDRESS;
 
+    config->mem_ops.alloc = mem_alloc;
+    config->mem_ops.v2p = virt_to_phys;
+
     acpi_build_tables(config, physical);
 
     hvm_param_set(HVM_PARAM_VM_GENERATION_ID_ADDR, config->acpi_info.vm_gid_addr);
diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
index b226df4..91c210c 100644
--- a/tools/firmware/hvmloader/util.h
+++ b/tools/firmware/hvmloader/util.h
@@ -196,7 +196,7 @@  xen_pfn_t mem_hole_alloc(uint32_t nr_mfns);
 
 /* Allocate memory in a reserved region below 4GB. */
 void *mem_alloc(uint32_t size, uint32_t align);
-#define virt_to_phys(v) ((unsigned long)(v))
+unsigned long virt_to_phys(void *v);
 
 /* Allocate memory in a scratch region */
 void *scratch_alloc(uint32_t size, uint32_t align);