Re: [PATCH 03/04] qemu-kvm: Remove the dependency for phys_ram_base

[Jes Sorensen <jes@sgi.com>]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

Avi Kivity wrote:
> Jes Sorensen wrote:
>> Hi,
>>
>> I am not crazy about this patch. You need to use cpy_physical_memory_rw()
>> in the hob and nvram code too, not just in the ipf.c code.
>>
>> What about the flush_icache_range() call you removed - is it safe to
>> just discard that?
>>
>> I was in the process of working through this myself, but I am not
>> quite finished. If you don't mind waiting a couple hours, I should
>> have something a fair bit simpler to solve the same problem.
>>
>> Biggest issue is the flush_icache_range() one.
>>
> I haven't pushed this out yet, so I can apply a replacement patch.

Hi,

As promised here is my proposal for how to implement this patch. I have
made the hob and nvram code use cpu_physical_memory_{read,write} as
well, handled the cache flushing address pointer issue in the flush
function, which needs some improvement to be done later.

By doing this I have been able to remove a bunch of globals and a number
of variables being passed around which we really don't need.

Xiantao, what do you think of this solution?

Cheers,
Jes



[-- Attachment #2: 0100-qemu-ia64-cpu-memory-write.patch --]
[-- Type: text/x-patch, Size: 10366 bytes --]

Remove ia64 dependency on phys_ram_base and assumption that
phys_ram_base + qemu_alloc_ram() is always valid. Use
cpu_physical_memory_{read,write} instead of memcpy.

The behavior of the NVRAM code is questionable, but this code should
behave the same as the old code ... it still needs to be fixed.

flush_icache_range() should also be improved to reget the host pointer
on every page boundary, but at least this is no worse than what we had
before.

Signed-off-by: Jes Sorensen <jes@sgi.com>

---
 hw/ipf.c                |   48 ++++++++++++------------------
 target-ia64/fake-exec.c |   16 ++++++++--
 target-ia64/firmware.c  |   76 +++++++++++++++++++++++++++++++-----------------
 target-ia64/firmware.h  |    8 +----
 4 files changed, 85 insertions(+), 63 deletions(-)

Index: qemu-kvm/hw/ipf.c
===================================================================
--- qemu-kvm.orig/hw/ipf.c
+++ qemu-kvm/hw/ipf.c
@@ -54,7 +54,6 @@
 static RTCState *rtc_state;
 static PCIDevice *i440fx_state;
 
-uint8_t *g_fw_start;
 static uint32_t ipf_to_legacy_io(target_phys_addr_t addr)
 {
     return (uint32_t)(((addr&0x3ffffff) >> 12 << 2)|((addr) & 0x3));
@@ -453,16 +452,14 @@
     /*Load firware to its proper position.*/
     if (kvm_enabled()) {
         unsigned long  image_size;
-        char *image = NULL;
-        uint8_t *fw_image_start;
+        uint8_t *image = NULL;
         unsigned long nvram_addr = 0;
         unsigned long nvram_fd = 0;
         unsigned long type = READ_FROM_NVRAM;
         unsigned long i = 0;
-        ram_addr_t fw_offset = qemu_ram_alloc(GFW_SIZE);
-        uint8_t *fw_start = phys_ram_base + fw_offset;
+        unsigned long fw_offset;
+        ram_addr_t fw_mem = qemu_ram_alloc(GFW_SIZE);
 
-        g_fw_start = fw_start;
         snprintf(buf, sizeof(buf), "%s/%s", bios_dir, FW_FILENAME);
         image = read_image(buf, &image_size );
         if (NULL == image || !image_size) {
@@ -470,28 +467,28 @@
             fprintf(stderr, "Please check Guest firmware at %s\n", buf);
             exit(1);
         }
-        fw_image_start = fw_start + GFW_SIZE - image_size;
+        fw_offset = GFW_START + GFW_SIZE - image_size;
 
-        cpu_register_physical_memory(GFW_START, GFW_SIZE, fw_offset);
-        memcpy(fw_image_start, image, image_size);
+        cpu_register_physical_memory(GFW_START, GFW_SIZE, fw_mem);
+        cpu_physical_memory_write(fw_offset, image, image_size);
 
         free(image);
-        flush_icache_range((unsigned long)fw_image_start,
-                           (unsigned long)fw_image_start + image_size);
+
+        flush_icache_range(fw_offset, fw_offset + image_size);
 
         nvram_addr = NVRAM_START;
         if (nvram) {
             nvram_fd = kvm_ia64_nvram_init(type);
             if (nvram_fd != -1) {
-                kvm_ia64_copy_from_nvram_to_GFW(nvram_fd, g_fw_start);
+                kvm_ia64_copy_from_nvram_to_GFW(nvram_fd);
                 close(nvram_fd);
             }
             i = atexit((void *)kvm_ia64_copy_from_GFW_to_nvram);
             if (i != 0)
                 fprintf(stderr, "cannot set exit function\n");
         }
-        kvm_ia64_build_hob(ram_size + above_4g_mem_size, smp_cpus,
-                           fw_start, nvram_addr);
+
+        kvm_ia64_build_hob(ram_size + above_4g_mem_size, smp_cpus, nvram_addr);
     }
 
     /*Register legacy io address space, size:64M*/
@@ -512,21 +509,15 @@
     }
 
     if (cirrus_vga_enabled) {
-        if (pci_enabled) {
-            pci_cirrus_vga_init(pci_bus, phys_ram_base + vga_ram_addr,
-                                vga_ram_addr, vga_ram_size);
-        } else {
-            isa_cirrus_vga_init(phys_ram_base + vga_ram_addr,
-                                vga_ram_addr, vga_ram_size);
-        }
+        if (pci_enabled)
+            pci_cirrus_vga_init(pci_bus, vga_ram_size);
+        else
+            isa_cirrus_vga_init(vga_ram_size);
     } else {
-        if (pci_enabled) {
-            pci_vga_init(pci_bus, phys_ram_base + vga_ram_addr,
-                         vga_ram_addr, vga_ram_size, 0, 0);
-        } else {
-            isa_vga_init(phys_ram_base + vga_ram_addr,
-                         vga_ram_addr, vga_ram_size);
-        }
+        if (pci_enabled)
+            pci_vga_init(pci_bus, vga_ram_size, 0, 0);
+        else
+            isa_vga_init(vga_ram_size);
     }
 
     rtc_state = rtc_init(0x70, i8259[8], 2000);
@@ -671,7 +662,6 @@
     .name = "itanium",
     .desc = "Itanium Platform",
     .init = (QEMUMachineInitFunc *)ipf_init_pci,
-    .ram_require = (ram_addr_t)(VGA_RAM_SIZE + GFW_SIZE),
     .max_cpus = 255,
 };
 
Index: qemu-kvm/target-ia64/fake-exec.c
===================================================================
--- qemu-kvm.orig/target-ia64/fake-exec.c
+++ qemu-kvm/target-ia64/fake-exec.c
@@ -43,9 +43,19 @@
 
 void flush_icache_range(unsigned long start, unsigned long stop)
 {
-    while (start < stop) {
-	asm volatile ("fc %0" :: "r"(start));
-	start += 32;
+    unsigned long size = stop - start;
+    unsigned long host_start, host_stop;
+
+    /*
+     * This probably should be made more advanced and regrab the pointer
+     * on every page boundary.
+     */
+    host_start = qemu_get_ram_ptr(start);
+    host_end = host_start + size;
+
+    while (host_start < host_stop) {
+	asm volatile ("fc %0" :: "r"(host_start));
+	host_start += 32;
     }
     asm volatile (";;sync.i;;srlz.i;;");
 }
Index: qemu-kvm/target-ia64/firmware.c
===================================================================
--- qemu-kvm.orig/target-ia64/firmware.c
+++ qemu-kvm/target-ia64/firmware.c
@@ -91,12 +91,11 @@
 static int build_hob(void *hob_buf, unsigned long hob_buf_size,
                      unsigned long dom_mem_size, unsigned long vcpus,
                      unsigned long nvram_addr);
-static int load_hob(void *hob_buf,
-                    unsigned long dom_mem_size, void* hob_start);
+static int load_hob(void *hob_buf, unsigned long dom_mem_size);
 
 int
 kvm_ia64_build_hob(unsigned long memsize, unsigned long vcpus,
-                   uint8_t *fw_start, unsigned long nvram_addr)
+                   unsigned long nvram_addr)
 {
     char   *hob_buf;
 
@@ -111,7 +110,8 @@
         Hob_Output("Could not build hob");
         return -1;
     }
-    if (load_hob(hob_buf, memsize, fw_start + HOB_OFFSET) < 0) {
+
+    if (load_hob(hob_buf, memsize) < 0) {
         free(hob_buf);
         Hob_Output("Could not load hob");
         return -1;
@@ -249,7 +249,7 @@
     return -1;
 }
 static int
-load_hob(void *hob_buf, unsigned long dom_mem_size, void* hob_start)
+load_hob(void *hob_buf, unsigned long dom_mem_size)
 {
     int hob_size;
 
@@ -263,7 +263,9 @@
         Hob_Output("No enough memory for hob data");
         return -1;
     }
-    memcpy (hob_start, hob_buf, hob_size);
+
+    cpu_physical_memory_write(GFW_HOB_START, hob_buf, hob_size);
+
     return 0;
 }
 
@@ -526,11 +528,11 @@
     return 0;
 }
 
-char *read_image(const char *filename, unsigned long *size)
+uint8_t *read_image(const char *filename, unsigned long *size)
 {
     int kernel_fd = -1;
     gzFile kernel_gfd = NULL;
-    char *image = NULL, *tmp;
+    uint8_t *image = NULL, *tmp;
     unsigned int bytes;
 
     if ((filename == NULL) || (size == NULL))
@@ -643,41 +645,63 @@
 }
 
 int
-kvm_ia64_copy_from_nvram_to_GFW(unsigned long nvram_fd,
-                                const uint8_t *fw_start)
+kvm_ia64_copy_from_nvram_to_GFW(unsigned long nvram_fd)
 {
     struct stat file_stat;
+    uint8_t *nvram_buf;
+    int r = 0;
+
+    nvram_buf = malloc(NVRAM_SIZE);
+
     if ((fstat(nvram_fd, &file_stat) < 0) ||
         (NVRAM_SIZE  != file_stat.st_size) ||
-        (read(nvram_fd, (void *)(fw_start + NVRAM_OFFSET),
-              NVRAM_SIZE) != NVRAM_SIZE))
-        return -1;
-    return 0;
+        (read(nvram_fd, nvram_buf, NVRAM_SIZE) != NVRAM_SIZE)) {
+        r = -1;
+        goto out;
+    }
+
+    cpu_physical_memory_write(NVRAM_START, nvram_buf, NVRAM_SIZE);
+
+ out:
+    free(nvram_buf);
+    return r;
 }
 
 int
 kvm_ia64_copy_from_GFW_to_nvram()
 {
+    struct nvram_save_addr nvram_addr_buf;
+    uint8_t *nvram_buf;
     unsigned long nvram_fd;
     unsigned long type = WRITE_TO_NVRAM;
-    unsigned long *nvram_addr = (unsigned long *)(g_fw_start + NVRAM_OFFSET);
+    int ret = -1;
+
+    nvram_buf = malloc(NVRAM_SIZE);
+    if (!nvram_buf)
+        goto out_free;
+
+    cpu_physical_memory_read(NVRAM_START, (uint8_t *)&nvram_addr_buf,
+                             sizeof(struct nvram_save_addr));
+    if (nvram_addr_buf.signature != NVRAM_VALID_SIG) {
+        goto out_free;
+    }
+
+    cpu_physical_memory_read(nvram_addr_buf.addr, nvram_buf, NVRAM_SIZE);
+
     nvram_fd = kvm_ia64_nvram_init(type);
     if (nvram_fd  == -1)
         goto out;
-    if (((struct nvram_save_addr *)nvram_addr)->signature != NVRAM_VALID_SIG) {
-        close(nvram_fd);
-        goto out;
-    }
+
     lseek(nvram_fd, 0, SEEK_SET);
-    if (write(nvram_fd, ((void *)(((struct nvram_save_addr *)nvram_addr)->addr +
-        (char *)phys_ram_base)), NVRAM_SIZE) != NVRAM_SIZE) {
-        close(nvram_fd);
+    if (write(nvram_fd, nvram_buf, NVRAM_SIZE) != NVRAM_SIZE)
         goto out;
-    }
+
+    ret = 0;
+ out:
     close(nvram_fd);
-    return 0;
-out:
-    return -1;
+ out_free:
+    free(nvram_buf);
+    return ret;
 }
 
 /*
Index: qemu-kvm/target-ia64/firmware.h
===================================================================
--- qemu-kvm.orig/target-ia64/firmware.h
+++ qemu-kvm/target-ia64/firmware.h
@@ -52,13 +52,11 @@
 };
 
 extern const char *nvram;
-extern uint8_t *g_fw_start;
 extern int kvm_ia64_build_hob(unsigned long memsize, unsigned long vcpus,
-                              uint8_t *fw_start, unsigned long nvram_addr);
-extern char *read_image(const char *filename, unsigned long *size);
+                              unsigned long nvram_addr);
+extern uint8_t *read_image(const char *filename, unsigned long *size);
 
 extern int kvm_ia64_copy_from_GFW_to_nvram(void);
 extern int kvm_ia64_nvram_init(unsigned long type);
-extern int kvm_ia64_copy_from_nvram_to_GFW(unsigned long nvram_fd,
-                                           const uint8_t *fw_start);
+extern int kvm_ia64_copy_from_nvram_to_GFW(unsigned long nvram_fd);
 #endif //__FIRM_WARE_