From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jes Sorensen Date: Tue, 28 Apr 2009 15:02:59 +0000 Subject: Re: [PATCH 03/04] qemu-kvm: Remove the dependency for phys_ram_base Message-Id: <49F71AA3.2040905@sgi.com> MIME-Version: 1 Content-Type: multipart/mixed; boundary="------------010206030801040005020603" List-Id: References: <706158FABBBA044BAD4FE898A02E4BC236A2BC04@pdsmsx503.ccr.corp.intel.com> In-Reply-To: <706158FABBBA044BAD4FE898A02E4BC236A2BC04@pdsmsx503.ccr.corp.intel.com> To: kvm-ia64@vger.kernel.org This is a multi-part message in MIME format. --------------010206030801040005020603 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 --------------010206030801040005020603 Content-Type: text/x-patch; name="0100-qemu-ia64-cpu-memory-write.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="0100-qemu-ia64-cpu-memory-write.patch" 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 --- 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_ --------------010206030801040005020603--