diff mbox

[v3,07/14] loader: support for unmapped ROM blobs

Message ID 1374681580-17439-8-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin July 24, 2013, 4:01 p.m. UTC 
Support ROM blobs not mapped into guest memory:
let user pass in MR for memory serving as the backing store.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/core/loader.c       | 32 +++++++++++++++++++++++++++++---
 hw/lm32/lm32_hwsetup.h |  2 +-
 include/hw/loader.h    |  4 ++--
 3 files changed, 32 insertions(+), 6 deletions(-)

Comments

Gerd Hoffmann July 25, 2013, 12:14 p.m. UTC | #1 
On 07/24/13 18:01, Michael S. Tsirkin wrote:
>      QTAILQ_FOREACH(rom, &roms, next) {
> +        if (rom->mr) {
> +            continue;
> +        }
>          if (rom->fw_file) {
>              continue;
>          }
>          if (rom->data == NULL) {
>              continue;
>          }
> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> +        if (rom->mr) {
> +            void *host = memory_region_get_ram_ptr(rom->mr);
> +            memcpy(host, rom->data, rom->datasize);

I think this code never ever runs ...

cheers,
  Gerd
Michael S. Tsirkin July 25, 2013, 12:28 p.m. UTC | #2 
On Thu, Jul 25, 2013 at 02:14:40PM +0200, Gerd Hoffmann wrote:
> On 07/24/13 18:01, Michael S. Tsirkin wrote:
> >      QTAILQ_FOREACH(rom, &roms, next) {
> > +        if (rom->mr) {
> > +            continue;
> > +        }
> >          if (rom->fw_file) {
> >              continue;
> >          }
> >          if (rom->data == NULL) {
> >              continue;
> >          }
> > -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> > +        if (rom->mr) {
> > +            void *host = memory_region_get_ram_ptr(rom->mr);
> > +            memcpy(host, rom->data, rom->datasize);
> 
> I think this code never ever runs ...
> 
> cheers,
>   Gerd
> 


Could you be clearer please? This chunk is in rom_reset,
I think it runs on reset.
Gerd Hoffmann July 25, 2013, 12:43 p.m. UTC | #3 
On 07/25/13 14:28, Michael S. Tsirkin wrote:
> On Thu, Jul 25, 2013 at 02:14:40PM +0200, Gerd Hoffmann wrote:
>> On 07/24/13 18:01, Michael S. Tsirkin wrote:
>>>      QTAILQ_FOREACH(rom, &roms, next) {
>>> +        if (rom->mr) {
>>> +            continue;
>>> +        }
>>>          if (rom->fw_file) {
>>>              continue;
>>>          }
>>>          if (rom->data == NULL) {
>>>              continue;
>>>          }
>>> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
>>> +        if (rom->mr) {
>>> +            void *host = memory_region_get_ram_ptr(rom->mr);
>>> +            memcpy(host, rom->data, rom->datasize);
>>
>> I think this code never ever runs ...
>>
>> cheers,
>>   Gerd
>>
> 
> 
> Could you be clearer please? This chunk is in rom_reset,
> I think it runs on reset.

You have the "if (rom->mr)" twice in the loop.  The first does continue
so the second will never ever evaluate to true, thereby making the
memcpy dead code.

cheers,
  Gerd
Michael S. Tsirkin July 25, 2013, 1:03 p.m. UTC | #4 
On Thu, Jul 25, 2013 at 02:43:53PM +0200, Gerd Hoffmann wrote:
> On 07/25/13 14:28, Michael S. Tsirkin wrote:
> > On Thu, Jul 25, 2013 at 02:14:40PM +0200, Gerd Hoffmann wrote:
> >> On 07/24/13 18:01, Michael S. Tsirkin wrote:
> >>>      QTAILQ_FOREACH(rom, &roms, next) {
> >>> +        if (rom->mr) {
> >>> +            continue;
> >>> +        }
> >>>          if (rom->fw_file) {
> >>>              continue;
> >>>          }
> >>>          if (rom->data == NULL) {
> >>>              continue;
> >>>          }
> >>> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> >>> +        if (rom->mr) {
> >>> +            void *host = memory_region_get_ram_ptr(rom->mr);
> >>> +            memcpy(host, rom->data, rom->datasize);
> >>
> >> I think this code never ever runs ...
> >>
> >> cheers,
> >>   Gerd
> >>
> > 
> > 
> > Could you be clearer please? This chunk is in rom_reset,
> > I think it runs on reset.
> 
> You have the "if (rom->mr)" twice in the loop.  The first does continue
> so the second will never ever evaluate to true, thereby making the
> memcpy dead code.
> 
> cheers,
>   Gerd


Hmm that's a bugt I think. Thanks!
Michael S. Tsirkin July 25, 2013, 7:57 p.m. UTC | #5 
On Thu, Jul 25, 2013 at 02:43:53PM +0200, Gerd Hoffmann wrote:
> On 07/25/13 14:28, Michael S. Tsirkin wrote:
> > On Thu, Jul 25, 2013 at 02:14:40PM +0200, Gerd Hoffmann wrote:
> >> On 07/24/13 18:01, Michael S. Tsirkin wrote:
> >>>      QTAILQ_FOREACH(rom, &roms, next) {
> >>> +        if (rom->mr) {
> >>> +            continue;
> >>> +        }
> >>>          if (rom->fw_file) {
> >>>              continue;
> >>>          }
> >>>          if (rom->data == NULL) {
> >>>              continue;
> >>>          }
> >>> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
> >>> +        if (rom->mr) {
> >>> +            void *host = memory_region_get_ram_ptr(rom->mr);
> >>> +            memcpy(host, rom->data, rom->datasize);
> >>
> >> I think this code never ever runs ...
> >>
> >> cheers,
> >>   Gerd
> >>
> > 
> > 
> > Could you be clearer please? This chunk is in rom_reset,
> > I think it runs on reset.
> 
> You have the "if (rom->mr)" twice in the loop.  The first does continue
> so the second will never ever evaluate to true, thereby making the
> memcpy dead code.
> 
> cheers,
>   Gerd
> 

Ow, good catch. I'll fix that up, thanks!
diff mbox

Patch

diff --git a/hw/core/loader.c b/hw/core/loader.c
index c4dd665..c2309e6 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -576,6 +576,7 @@  struct Rom {
     size_t datasize;
 
     uint8_t *data;
+    MemoryRegion *mr;
     int isrom;
     char *fw_dir;
     char *fw_file;
@@ -675,7 +676,7 @@  err:
 }
 
 int rom_add_blob(const char *name, const void *blob, size_t len,
-                 hwaddr addr)
+                 hwaddr addr, MemoryRegion *mr)
 {
     Rom *rom;
 
@@ -685,6 +686,11 @@  int rom_add_blob(const char *name, const void *blob, size_t len,
     rom->romsize  = len;
     rom->datasize = len;
     rom->data     = g_malloc0(rom->datasize);
+    rom->mr       = mr;
+    if (mr) {
+        assert(memory_region_is_ram(mr));
+        rom->isrom = memory_region_is_rom(mr);
+    }
     memcpy(rom->data, blob, len);
     rom_insert(rom);
     return 0;
@@ -725,13 +731,21 @@  static void rom_reset(void *unused)
     Rom *rom;
 
     QTAILQ_FOREACH(rom, &roms, next) {
+        if (rom->mr) {
+            continue;
+        }
         if (rom->fw_file) {
             continue;
         }
         if (rom->data == NULL) {
             continue;
         }
-        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
+        if (rom->mr) {
+            void *host = memory_region_get_ram_ptr(rom->mr);
+            memcpy(host, rom->data, rom->datasize);
+        } else {
+            cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
+        }
         if (rom->isrom) {
             /* rom needs to be written only once */
             g_free(rom->data);
@@ -781,6 +795,9 @@  static Rom *find_rom(hwaddr addr)
         if (rom->fw_file) {
             continue;
         }
+        if (rom->mr) {
+            continue;
+        }
         if (rom->addr > addr) {
             continue;
         }
@@ -808,6 +825,9 @@  int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
         if (rom->fw_file) {
             continue;
         }
+        if (rom->mr) {
+            continue;
+        }
         if (rom->addr + rom->romsize < addr) {
             continue;
         }
@@ -867,7 +887,13 @@  void do_info_roms(Monitor *mon, const QDict *qdict)
     Rom *rom;
 
     QTAILQ_FOREACH(rom, &roms, next) {
-        if (!rom->fw_file) {
+        if (rom->mr) {
+            monitor_printf(mon, "%s"
+                           " size=0x%06zx name=\"%s\"\n",
+                           rom->mr->name,
+                           rom->romsize,
+                           rom->name);
+        } else if (!rom->fw_file) {
             monitor_printf(mon, "addr=" TARGET_FMT_plx
                            " size=0x%06zx mem=%s name=\"%s\"\n",
                            rom->addr, rom->romsize,
diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h
index 3449bd8..d6914d6 100644
--- a/hw/lm32/lm32_hwsetup.h
+++ b/hw/lm32/lm32_hwsetup.h
@@ -73,7 +73,7 @@  static inline void hwsetup_free(HWSetup *hw)
 static inline void hwsetup_create_rom(HWSetup *hw,
         hwaddr base)
 {
-    rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE, base);
+    rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE, base, NULL);
 }
 
 static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u)
diff --git a/include/hw/loader.h b/include/hw/loader.h
index eb9c9a3..cdb7b4b 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -40,7 +40,7 @@  void pstrcpy_targphys(const char *name,
 int rom_add_file(const char *file, const char *fw_dir,
                  hwaddr addr, int32_t bootindex);
 int rom_add_blob(const char *name, const void *blob, size_t len,
-                 hwaddr addr);
+                 hwaddr addr, MemoryRegion *mr);
 int rom_add_elf_program(const char *name, void *data, size_t datasize,
                         size_t romsize, hwaddr addr);
 int rom_load_all(void);
@@ -52,7 +52,7 @@  void do_info_roms(Monitor *mon, const QDict *qdict);
 #define rom_add_file_fixed(_f, _a, _i)          \
     rom_add_file(_f, NULL, _a, _i)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
-    rom_add_blob(_f, _b, _l, _a)
+    rom_add_blob(_f, _b, _l, _a, NULL)
 
 #define PC_ROM_MIN_VGA     0xc0000
 #define PC_ROM_MIN_OPTION  0xc8000