diff mbox

[03/04] qemu-kvm: Remove the dependency for phys_ram_base for ipf.c

Message ID 706158FABBBA044BAD4FE898A02E4BC236A2BC04@pdsmsx503.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhang, Xiantao April 28, 2009, 9:29 a.m. UTC
From aaf97331da3d6cd34522441218c8c9ab3c1067f6 Mon Sep 17 00:00:00 2001
From: Xiantao Zhang <xiantao.zhang@intel.com>
Date: Tue, 28 Apr 2009 16:55:47 +0800
Subject: [PATCH] qemu-kvm: Remove the dependency for phys_ram_base for ipf.c
 
Upstream has dropped phys_ram_base, so ia64 also remove
the dependency for that.
 
Signed-off-by: Xiantao Zhang <xiantao.zhang@intel.com>
---
 hw/ipf.c               |   42 +++++++++++++++++++-----------------------
 target-ia64/firmware.c |   25 +++++++++++++++++--------
 target-ia64/firmware.h |    6 +++---
 3 files changed, 39 insertions(+), 34 deletions(-)

Comments

Avi Kivity April 28, 2009, 9:49 a.m. UTC | #1
Zhang, Xiantao wrote:
> From aaf97331da3d6cd34522441218c8c9ab3c1067f6 Mon Sep 17 00:00:00 2001
> From: Xiantao Zhang <xiantao.zhang@intel.com>
> Date: Tue, 28 Apr 2009 16:55:47 +0800
> Subject: [PATCH] qemu-kvm: Remove the dependency for phys_ram_base for ipf.c
>  
> Upstream has dropped phys_ram_base, so ia64 also remove
> the dependency for that.
>  
> +++ b/hw/ipf.c
> @@ -54,7 +54,8 @@ static fdctrl_t *floppy_controller;
>  static RTCState *rtc_state;
>  static PCIDevice *i440fx_state;
>  
> -uint8_t *g_fw_start;
> +ram_addr_t gfw_start;
> +
>  static uint32_t ipf_to_legacy_io(target_phys_addr_t addr)
>  {
>      return (uint32_t)(((addr&0x3ffffff) >> 12 << 2)|((addr) & 0x3));
> @@ -454,15 +455,15 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
>      if (kvm_enabled()) {
>          unsigned long  image_size;
>          char *image = NULL;
> -        uint8_t *fw_image_start;
> +        ram_addr_t fw_image_start;
>          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;
>  
> -        g_fw_start = fw_start;
> +        ram_addr  = qemu_ram_alloc(GFW_SIZE);
> +        gfw_start = (ram_addr_t)qemu_get_ram_ptr(ram_addr);
>   

qemu_get_ram_ptr() returns a pointer.  Don't cast it to a ram_addr_t, 
leave it a pointer.

But why not use cpu_physical_memory_write() (or 
cpu_physical_memory_write_rom())?  It's much simpler and cleaner.
Zhang, Xiantao April 28, 2009, 10:36 a.m. UTC | #2
Avi Kivity wrote:
> Zhang, Xiantao wrote:
>> From aaf97331da3d6cd34522441218c8c9ab3c1067f6 Mon Sep 17 00:00:00
>> 2001 
>> From: Xiantao Zhang <xiantao.zhang@intel.com>
>> Date: Tue, 28 Apr 2009 16:55:47 +0800
>> Subject: [PATCH] qemu-kvm: Remove the dependency for phys_ram_base
>> for ipf.c 
>> 
>> Upstream has dropped phys_ram_base, so ia64 also remove
>> the dependency for that.
>> 
>> +++ b/hw/ipf.c
>> @@ -54,7 +54,8 @@ static fdctrl_t *floppy_controller;  static
>>  RTCState *rtc_state; static PCIDevice *i440fx_state;
>> 
>> -uint8_t *g_fw_start;
>> +ram_addr_t gfw_start;
>> +
>>  static uint32_t ipf_to_legacy_io(target_phys_addr_t addr)  {
>>      return (uint32_t)(((addr&0x3ffffff) >> 12 << 2)|((addr) & 0x3));
>> @@ -454,15 +455,15 @@ static void ipf_init1(ram_addr_t ram_size, int
>>          vga_ram_size,      if (kvm_enabled()) { unsigned long 
>>          image_size; char *image = NULL;
>> -        uint8_t *fw_image_start;
>> +        ram_addr_t fw_image_start;
>>          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;
>> 
>> -        g_fw_start = fw_start;
>> +        ram_addr  = qemu_ram_alloc(GFW_SIZE);
>> +        gfw_start = (ram_addr_t)qemu_get_ram_ptr(ram_addr);
>> 
> 
> qemu_get_ram_ptr() returns a pointer.  Don't cast it to a ram_addr_t,
> leave it a pointer.
> 
> But why not use cpu_physical_memory_write() (or
> cpu_physical_memory_write_rom())?  It's much simpler and cleaner.

Good suggestion! I just followed the original logic.  Updated the patch. 
Xiantao
Avi Kivity April 28, 2009, 11:05 a.m. UTC | #3
Zhang, Xiantao wrote:
>>
>> qemu_get_ram_ptr() returns a pointer.  Don't cast it to a ram_addr_t,
>> leave it a pointer.
>>
>> But why not use cpu_physical_memory_write() (or
>> cpu_physical_memory_write_rom())?  It's much simpler and cleaner.
>>     
>
> Good suggestion! I just followed the original logic.  Updated the patch. 
> Xiantao

Thanks, applied.
Jes Sorensen April 28, 2009, 12:38 p.m. UTC | #4
>>>>> "Avi" == Avi Kivity <avi@redhat.com> writes:

Avi> Zhang, Xiantao wrote:
>>>  qemu_get_ram_ptr() returns a pointer.  Don't cast it to a
>>> ram_addr_t, leave it a pointer.
>>> 
>>> But why not use cpu_physical_memory_write() (or
>>> cpu_physical_memory_write_rom())?  It's much simpler and cleaner.
>>> 
>>  Good suggestion! I just followed the original logic.  Updated the
>> patch. Xiantao

Avi> Thanks, applied.

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.

Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Avi Kivity April 28, 2009, 12:49 p.m. UTC | #5
Jes Sorensen wrote:
>>>>>> "Avi" == Avi Kivity <avi@redhat.com> writes:
>>>>>>             
>
> Avi> Zhang, Xiantao wrote:
>   
>>>>  qemu_get_ram_ptr() returns a pointer.  Don't cast it to a
>>>> ram_addr_t, leave it a pointer.
>>>>
>>>> But why not use cpu_physical_memory_write() (or
>>>> cpu_physical_memory_write_rom())?  It's much simpler and cleaner.
>>>>
>>>>         
>>>  Good suggestion! I just followed the original logic.  Updated the
>>> patch. Xiantao
>>>       
>
> Avi> Thanks, applied.
>
> 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.
Zhang, Xiantao April 29, 2009, 2:04 a.m. UTC | #6
Jes Sorensen wrote:
>>>>>> "Avi" == Avi Kivity <avi@redhat.com> writes:
> 
> Avi> Zhang, Xiantao wrote:
>>>>  qemu_get_ram_ptr() returns a pointer.  Don't cast it to a
>>>> ram_addr_t, leave it a pointer.
>>>> 
>>>> But why not use cpu_physical_memory_write() (or
>>>> cpu_physical_memory_write_rom())?  It's much simpler and cleaner.
>>>> 
>>>  Good suggestion! I just followed the original logic.  Updated the
>>> patch. Xiantao
> 
> Avi> Thanks, applied.
> 
> Hi,

Hi, Jes

> 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. 

Agree, maybe you can make an increment patch for that.

> What about the flush_icache_range() call you removed - is it safe to
> just discard that?"

Yes, I think cpu_physical_memory_write also called the flush_icache_range, so don't need to duplicate it. 

> 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.
> 
> Cheers,
> Jes

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang, Xiantao April 29, 2009, 2:05 a.m. UTC | #7
Avi Kivity wrote:
> Jes Sorensen wrote:
>>>>>>> "Avi" == Avi Kivity <avi@redhat.com> writes:
>>>>>>> 
>> 
>> Avi> Zhang, Xiantao wrote:
>> 
>>>>>  qemu_get_ram_ptr() returns a pointer.  Don't cast it to a
>>>>> ram_addr_t, leave it a pointer.
>>>>> 
>>>>> But why not use cpu_physical_memory_write() (or
>>>>> cpu_physical_memory_write_rom())?  It's much simpler and cleaner.
>>>>> 
>>>>> 
>>>>  Good suggestion! I just followed the original logic.  Updated the
>>>> patch. Xiantao 
>>>> 
>> 
>> Avi> Thanks, applied.
>> 
>> 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. 

We don't need flush_icache_range here, because I believe it is called in cpu_physical_memory_write. 
Xiantao--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/hw/ipf.c b/hw/ipf.c
index 5ed2667..ccd7665 100644
--- a/hw/ipf.c
+++ b/hw/ipf.c
@@ -54,7 +54,8 @@  static fdctrl_t *floppy_controller;
 static RTCState *rtc_state;
 static PCIDevice *i440fx_state;
 
-uint8_t *g_fw_start;
+ram_addr_t gfw_start;
+
 static uint32_t ipf_to_legacy_io(target_phys_addr_t addr)
 {
     return (uint32_t)(((addr&0x3ffffff) >> 12 << 2)|((addr) & 0x3));
@@ -454,15 +455,15 @@  static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
     if (kvm_enabled()) {
         unsigned long  image_size;
         char *image = NULL;
-        uint8_t *fw_image_start;
+        ram_addr_t fw_image_start;
         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;
 
-        g_fw_start = fw_start;
+        ram_addr  = qemu_ram_alloc(GFW_SIZE);
+        gfw_start = (ram_addr_t)qemu_get_ram_ptr(ram_addr);
+
         snprintf(buf, sizeof(buf), "%s/%s", bios_dir, FW_FILENAME);
         image = read_image(buf, &image_size );
         if (NULL == image || !image_size) {
@@ -470,20 +471,20 @@  static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
             fprintf(stderr, "Please check Guest firmware at %s\n", buf);
             exit(1);
         }
-        fw_image_start = fw_start + GFW_SIZE - image_size;
-
-        cpu_register_physical_memory(GFW_START, GFW_SIZE, fw_offset);
-        memcpy(fw_image_start, image, image_size);
-
-        free(image);
+ /* Load Guest Firmware to the proper postion. */
+        fw_image_start = gfw_start + GFW_SIZE - image_size;
+        memcpy((void *)fw_image_start, image, image_size);
         flush_icache_range((unsigned long)fw_image_start,
                            (unsigned long)fw_image_start + image_size);
+        free(image);
+
+        cpu_register_physical_memory(GFW_START, GFW_SIZE, ram_addr);
 
-        nvram_addr = NVRAM_START;
         if (nvram) {
+            nvram_addr = NVRAM_START;
             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, gfw_start);
                 close(nvram_fd);
             }
             i = atexit((void *)kvm_ia64_copy_from_GFW_to_nvram);
@@ -491,7 +492,7 @@  static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
                 fprintf(stderr, "cannot set exit function\n");
         }
         kvm_ia64_build_hob(ram_size + above_4g_mem_size, smp_cpus,
-                           fw_start, nvram_addr);
+                           gfw_start, nvram_addr);
     }
 
     /*Register legacy io address space, size:64M*/
@@ -513,19 +514,15 @@  static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
 
     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);
+            pci_cirrus_vga_init(pci_bus, vga_ram_size);
         } else {
-            isa_cirrus_vga_init(phys_ram_base + vga_ram_addr,
-                                vga_ram_addr, vga_ram_size);
+            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);
+            pci_vga_init(pci_bus, vga_ram_size, 0, 0);
         } else {
-            isa_vga_init(phys_ram_base + vga_ram_addr,
-                         vga_ram_addr, vga_ram_size);
+            isa_vga_init(vga_ram_size);
         }
     }
 
@@ -671,7 +668,6 @@  QEMUMachine ipf_machine = {
     .name = "itanium",
     .desc = "Itanium Platform",
     .init = (QEMUMachineInitFunc *)ipf_init_pci,
-    .ram_require = (ram_addr_t)(VGA_RAM_SIZE + GFW_SIZE),
     .max_cpus = 255,
 };
 
diff --git a/target-ia64/firmware.c b/target-ia64/firmware.c
index 87a8178..2ed2e0d 100644
--- a/target-ia64/firmware.c
+++ b/target-ia64/firmware.c
@@ -92,11 +92,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);
+                    unsigned long dom_mem_size, ram_addr_t hob_start);
 
 int
 kvm_ia64_build_hob(unsigned long memsize, unsigned long vcpus,
-                   uint8_t *fw_start, unsigned long nvram_addr)
+                   ram_addr_t fw_start, unsigned long nvram_addr)
 {
     char   *hob_buf;
 
@@ -249,7 +249,7 @@  err_out:
     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, ram_addr_t hob_start)
 {
     int hob_size;
 
@@ -263,7 +263,7 @@  load_hob(void *hob_buf, unsigned long dom_mem_size, void* hob_start)
         Hob_Output("No enough memory for hob data");
         return -1;
     }
-    memcpy (hob_start, hob_buf, hob_size);
+    memcpy ((void *)hob_start, hob_buf, hob_size);
     return 0;
 }
 
@@ -644,7 +644,7 @@  out:
 
 int
 kvm_ia64_copy_from_nvram_to_GFW(unsigned long nvram_fd,
-                                const uint8_t *fw_start)
+                                const ram_addr_t fw_start)
 {
     struct stat file_stat;
     if ((fstat(nvram_fd, &file_stat) < 0) ||
@@ -659,8 +659,13 @@  int
 kvm_ia64_copy_from_GFW_to_nvram()
 {
     unsigned long nvram_fd;
+    void* real_nvram_start;
+    target_phys_addr_t nvram_size = NVRAM_SIZE;
     unsigned long type = WRITE_TO_NVRAM;
-    unsigned long *nvram_addr = (unsigned long *)(g_fw_start + NVRAM_OFFSET);
+    unsigned long *nvram_addr = (unsigned long *)(gfw_start + NVRAM_OFFSET);
+
+
+
     nvram_fd = kvm_ia64_nvram_init(type);
     if (nvram_fd  == -1)
         goto out;
@@ -669,11 +674,15 @@  kvm_ia64_copy_from_GFW_to_nvram()
         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) {
+
+    real_nvram_start = cpu_physical_memory_map(((struct nvram_save_addr*)nvram_addr)->addr,
+ &nvram_size, 1);
+    if (write(nvram_fd, real_nvram_start, NVRAM_SIZE) != NVRAM_SIZE) {
         close(nvram_fd);
         goto out;
     }
+    cpu_physical_memory_unmap(real_nvram_start, NVRAM_SIZE, 1, NVRAM_SIZE);
+
     close(nvram_fd);
     return 0;
 out:
diff --git a/target-ia64/firmware.h b/target-ia64/firmware.h
index c1707ac..548352c 100644
--- a/target-ia64/firmware.h
+++ b/target-ia64/firmware.h
@@ -52,13 +52,13 @@  struct nvram_save_addr {
 };
 
 extern const char *nvram;
-extern uint8_t *g_fw_start;
+extern ram_addr_t gfw_start;
 extern int kvm_ia64_build_hob(unsigned long memsize, unsigned long vcpus,
-                              uint8_t *fw_start, unsigned long nvram_addr);
+                              ram_addr_t fw_start, unsigned long nvram_addr);
 extern char *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);
+                                           const ram_addr_t fw_start);
 #endif //__FIRM_WARE_