[Qemu-devel] phys_ram_base, direct access to guest memory

[Qemu-devel] phys_ram_base, direct access to guest memory

[Ian Jackson]

As I think has been mentioned here a few times before, Xen is able to
support guests with more RAM than the host's (strictly, dom0's)
address space.  For example, 64-bit guests with >4G RAM on 32-bit
hosts.  For this and for other reasons, guest physical RAM is not
mapped into any host process.

I don't expect qemu to take the Xen mapcache, which has been
extensively discussed and is apparently not well-regarded here.

However, it would be very helpful if where reasonable parts of qemu
would avoid assuming that they can get at guest physical memory by use
of phys_ram_base.

For example, in the loader in pc.c, simply adding phys_ram_base does
not work and we have to have a rather large patch to convert things to
use cpu_physical_memory_rw.  The result is no more cumbersome -
indeed, it's slightly tidier in a few ways because there's less need
to constantly add and subtract phys_ram_base; the code can just deal
with guest physical addresses directly, as numbers, and leave the
actual memory access to the existing physical memory abstraction.

So I think it would be nice to have that change in qemu upstream.
While it doesn't directly enable anything useful right away, the
result is slightly cleaner.  I'll submit a proper patch shortly.

Ian.

[Qemu-devel] [PATCH] Remove most uses of phys_ram_base in hw/pc.c

[Ian Jackson]

I wrote:
> So I think it would be nice to have that change in qemu upstream.
> While it doesn't directly enable anything useful right away, the
> result is slightly cleaner.  I'll submit a proper patch shortly.

In the attached patch, I remove all the direct uses of phys_ram_base
from hw/pc.c, except for those presently needed to construct the
arguments to the vga init functions.

This involved:
 * Getting rid of various additions and subtractions of phys_ram_base
 * Changing the types of the guest physical addresses in load_linux
   from uint8_t* to target_phys_addr_t
 * Replacing calls to memcpy and pstrcpy with
   cpu_physical_memory_write (and a new pstrcpy_targphys function)
 * Replacing most calls to fread with a new fread_targphys function
 * Deprecating load_image in favour of a new load_image_targphys
 * Removing (rather than fixing up) the unused function load_kernel

I noticed that load_image doesn't take a buffer size argument - it
just overwrites the destination buffer with file data, extending as
long as the file happens to be.  In most cases this is probably not an
exploitable vulnerability, but it seems poor practice.  Hence
load_image_targphys's extra argument.

I hope this meets with your approval.

Thanks,
Ian.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/pc.c  |   98 ++++++++++++++++++++++++++-----------------------------------
 loader.c |   43 +++++++++++++++++++++++++++
 sysemu.h |    7 ++++-
 3 files changed, 91 insertions(+), 57 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 5333f47..6a2d9ea 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -435,36 +435,6 @@ static void generate_bootsect(uint32_t gpr[8], uint16_t segs[6], uint16_t ip)
     bdrv_set_boot_sector(drives_table[hda].bdrv, bootsect, sizeof(bootsect));
 }
 
-static int load_kernel(const char *filename, uint8_t *addr,
-                       uint8_t *real_addr)
-{
-    int fd, size;
-    int setup_sects;
-
-    fd = open(filename, O_RDONLY | O_BINARY);
-    if (fd < 0)
-        return -1;
-
-    /* load 16 bit code */
-    if (qemu_read_ok(fd, real_addr, 512) < 0)
-        goto fail;
-    setup_sects = real_addr[0x1F1];
-    if (!setup_sects)
-        setup_sects = 4;
-    if (qemu_read_ok(fd, real_addr + 512, setup_sects * 512) < 0)
-        goto fail;
-
-    /* load 32 bit code */
-    size = qemu_read(fd, addr, 16 * 1024 * 1024);
-    if (size < 0)
-        goto fail;
-    close(fd);
-    return size;
- fail:
-    close(fd);
-    return -1;
-}
-
 static long get_file_size(FILE *f)
 {
     long where, size;
@@ -479,6 +449,22 @@ static long get_file_size(FILE *f)
     return size;
 }
 
+static void pstrcpy_targphys(target_phys_addr_t dest, int buf_size,
+			     const char *source)
+{
+    static const char nul_byte;
+    const char *nulp;
+
+    if (buf_size <= 0) return;
+    nulp = memchr(source, 0, buf_size);
+    if (nulp) {
+	cpu_physical_memory_write(dest, source, (nulp - source) + 1);
+    } else {
+	cpu_physical_memory_write(dest, source, buf_size - 1);
+	cpu_physical_memory_write(dest, &nul_byte, 1);
+    }
+}
+
 static void load_linux(const char *kernel_filename,
 		       const char *initrd_filename,
 		       const char *kernel_cmdline)
@@ -490,7 +476,7 @@ static void load_linux(const char *kernel_filename,
     int setup_size, kernel_size, initrd_size, cmdline_size;
     uint32_t initrd_max;
     uint8_t header[1024];
-    uint8_t *real_addr, *prot_addr, *cmdline_addr, *initrd_addr;
+    target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr;
     FILE *f, *fi;
 
     /* Align to 16 bytes as a paranoia measure */
@@ -516,19 +502,19 @@ static void load_linux(const char *kernel_filename,
 
     if (protocol < 0x200 || !(header[0x211] & 0x01)) {
 	/* Low kernel */
-	real_addr    = phys_ram_base + 0x90000;
-	cmdline_addr = phys_ram_base + 0x9a000 - cmdline_size;
-	prot_addr    = phys_ram_base + 0x10000;
+	real_addr    = 0x90000;
+	cmdline_addr = 0x9a000 - cmdline_size;
+	prot_addr    = 0x10000;
     } else if (protocol < 0x202) {
 	/* High but ancient kernel */
-	real_addr    = phys_ram_base + 0x90000;
-	cmdline_addr = phys_ram_base + 0x9a000 - cmdline_size;
-	prot_addr    = phys_ram_base + 0x100000;
+	real_addr    = 0x90000;
+	cmdline_addr = 0x9a000 - cmdline_size;
+	prot_addr    = 0x100000;
     } else {
 	/* High and recent kernel */
-	real_addr    = phys_ram_base + 0x10000;
-	cmdline_addr = phys_ram_base + 0x20000;
-	prot_addr    = phys_ram_base + 0x100000;
+	real_addr    = 0x10000;
+	cmdline_addr = 0x20000;
+	prot_addr    = 0x100000;
     }
 
 #if 0
@@ -536,9 +522,9 @@ static void load_linux(const char *kernel_filename,
 	    "qemu: real_addr     = %#zx\n"
 	    "qemu: cmdline_addr  = %#zx\n"
 	    "qemu: prot_addr     = %#zx\n",
-	    real_addr-phys_ram_base,
-	    cmdline_addr-phys_ram_base,
-	    prot_addr-phys_ram_base);
+	    real_addr,
+	    cmdline_addr,
+	    prot_addr);
 #endif
 
     /* highest address for loading the initrd */
@@ -551,10 +537,10 @@ static void load_linux(const char *kernel_filename,
 	initrd_max = ram_size-ACPI_DATA_SIZE-1;
 
     /* kernel command line */
-    pstrcpy((char*)cmdline_addr, 4096, kernel_cmdline);
+    pstrcpy_targphys(cmdline_addr, 4096, kernel_cmdline);
 
     if (protocol >= 0x202) {
-	stl_p(header+0x228, cmdline_addr-phys_ram_base);
+	stl_p(header+0x228, cmdline_addr);
     } else {
 	stw_p(header+0x20, 0xA33F);
 	stw_p(header+0x22, cmdline_addr-real_addr);
@@ -588,24 +574,24 @@ static void load_linux(const char *kernel_filename,
 	}
 
 	initrd_size = get_file_size(fi);
-	initrd_addr = phys_ram_base + ((initrd_max-initrd_size) & ~4095);
+	initrd_addr = ((initrd_max-initrd_size) & ~4095);
 
 	fprintf(stderr, "qemu: loading initrd (%#x bytes) at %#zx\n",
-		initrd_size, initrd_addr-phys_ram_base);
+		initrd_size, initrd_addr);
 
-	if (fread(initrd_addr, 1, initrd_size, fi) != initrd_size) {
+	if (!fread_targphys_ok(initrd_addr, initrd_size, fi)) {
 	    fprintf(stderr, "qemu: read error on initial ram disk '%s'\n",
 		    initrd_filename);
 	    exit(1);
 	}
 	fclose(fi);
 
-	stl_p(header+0x218, initrd_addr-phys_ram_base);
+	stl_p(header+0x218, initrd_addr);
 	stl_p(header+0x21c, initrd_size);
     }
 
     /* store the finalized header and load the rest of the kernel */
-    memcpy(real_addr, header, 1024);
+    cpu_physical_memory_write(real_addr, header, 1024);
 
     setup_size = header[0x1f1];
     if (setup_size == 0)
@@ -614,8 +600,8 @@ static void load_linux(const char *kernel_filename,
     setup_size = (setup_size+1)*512;
     kernel_size -= setup_size;	/* Size of protected-mode code */
 
-    if (fread(real_addr+1024, 1, setup_size-1024, f) != setup_size-1024 ||
-	fread(prot_addr, 1, kernel_size, f) != kernel_size) {
+    if (!fread_targphys_ok(real_addr+1024, setup_size-1024, f) ||
+	!fread_targphys_ok(prot_addr, kernel_size, f)) {
 	fprintf(stderr, "qemu: read error on kernel '%s'\n",
 		kernel_filename);
 	exit(1);
@@ -623,7 +609,7 @@ static void load_linux(const char *kernel_filename,
     fclose(f);
 
     /* generate bootsector to set up the initial register state */
-    real_seg = (real_addr-phys_ram_base) >> 4;
+    real_seg = real_addr >> 4;
     seg[0] = seg[2] = seg[3] = seg[4] = seg[4] = real_seg;
     seg[1] = real_seg+0x20;	/* CS */
     memset(gpr, 0, sizeof gpr);
@@ -764,7 +750,7 @@ static void pc_init1(int ram_size, int vga_ram_size,
         goto bios_error;
     }
     bios_offset = qemu_ram_alloc(bios_size);
-    ret = load_image(buf, phys_ram_base + bios_offset);
+    ret = load_image_targphys(buf, bios_offset, bios_size);
     if (ret != bios_size) {
     bios_error:
         fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", buf);
@@ -782,7 +768,7 @@ static void pc_init1(int ram_size, int vga_ram_size,
         goto vga_bios_error;
     vga_bios_offset = qemu_ram_alloc(65536);
 
-    ret = load_image(buf, phys_ram_base + vga_bios_offset);
+    ret = load_image_targphys(buf, vga_bios_offset, vga_bios_size);
     if (ret != vga_bios_size) {
     vga_bios_error:
         fprintf(stderr, "qemu: could not load VGA BIOS '%s'\n", buf);
@@ -818,7 +804,7 @@ static void pc_init1(int ram_size, int vga_ram_size,
             if (size > (0x10000 - offset))
                 goto option_rom_error;
             option_rom_offset = qemu_ram_alloc(size);
-            ret = load_image(option_rom[i], phys_ram_base + option_rom_offset);
+            ret = load_image_targphys(option_rom[i], option_rom_offset, size);
             if (ret != size) {
             option_rom_error:
                 fprintf(stderr, "Too many option ROMS\n");
diff --git a/loader.c b/loader.c
index 813756e..5462d24 100644
--- a/loader.c
+++ b/loader.c
@@ -39,6 +39,7 @@ int get_image_size(const char *filename)
 }
 
 /* return the size or -1 if error */
+/* deprecated, because caller does not specify buffer size! */
 int load_image(const char *filename, uint8_t *addr)
 {
     int fd, size;
@@ -55,6 +56,48 @@ int load_image(const char *filename, uint8_t *addr)
     return size;
 }
 
+/* return the amount read, just like fread.  0 may mean error or eof */
+int fread_targphys(target_phys_addr_t dst_addr, size_t nbytes, FILE *f)
+{
+    unsigned char buf[4096];
+    target_phys_addr_t dst_begin = dst_addr;
+    size_t want, did;
+
+    while (nbytes) {
+	want = nbytes > sizeof(buf) ? sizeof(buf) : nbytes;
+	did = fread(buf, 1, want, f);
+	if (did != want) break;
+	
+	cpu_physical_memory_write(dst_addr, buf, did);
+	dst_addr += did;
+	nbytes -= did;
+    }
+    return dst_addr - dst_begin;
+}
+
+/* returns 0 on error, 1 if ok */
+int fread_targphys_ok(target_phys_addr_t dst_addr, size_t nbytes, FILE *f)
+{
+    return fread_targphys(dst_addr, nbytes, f) == nbytes;
+}
+    
+/* return the size or -1 if error */
+int load_image_targphys(const char *filename,
+			target_phys_addr_t addr, int max_sz)
+{
+    FILE *f;
+    size_t got;
+
+    f = fopen(filename, "rb");
+    if (!f) return -1;
+
+    got = fread_targphys(addr, max_sz, f);
+    if (ferror(f)) { fclose(f); return -1; }
+    fclose(f);
+
+    return got;
+}
+
 /* A.OUT loader */
 
 struct exec
diff --git a/sysemu.h b/sysemu.h
index 296f179..e62f832 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -150,11 +150,16 @@ extern CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 #ifdef NEED_CPU_H
 /* loader.c */
 int get_image_size(const char *filename);
-int load_image(const char *filename, uint8_t *addr);
+int load_image(const char *filename, uint8_t *addr); /* deprecated */
+int load_image_targphys(const char *filename, target_phys_addr_t, int max_sz);
 int load_elf(const char *filename, int64_t virt_to_phys_addend,
              uint64_t *pentry, uint64_t *lowaddr, uint64_t *highaddr);
 int load_aout(const char *filename, uint8_t *addr);
 int load_uboot(const char *filename, target_ulong *ep, int *is_linux);
+
+int fread_targphys(target_phys_addr_t dst_addr, size_t nbytes, FILE *f);
+int fread_targphys_ok(target_phys_addr_t dst_addr, size_t nbytes, FILE *f);
+
 #endif
 
 #ifdef HAS_AUDIO
-- 
1.4.4.4

Re: [Qemu-devel] phys_ram_base, direct access to guest memory

[Avi Kivity]

Ian Jackson wrote:
> As I think has been mentioned here a few times before, Xen is able to
> support guests with more RAM than the host's (strictly, dom0's)
> address space.  For example, 64-bit guests with >4G RAM on 32-bit
> hosts.  For this and for other reasons, guest physical RAM is not
> mapped into any host process.
>
> I don't expect qemu to take the Xen mapcache, which has been
> extensively discussed and is apparently not well-regarded here.
>
> However, it would be very helpful if where reasonable parts of qemu
> would avoid assuming that they can get at guest physical memory by use
> of phys_ram_base.
>
> For example, in the loader in pc.c, simply adding phys_ram_base does
> not work and we have to have a rather large patch to convert things to
> use cpu_physical_memory_rw.  The result is no more cumbersome -
> indeed, it's slightly tidier in a few ways because there's less need
> to constantly add and subtract phys_ram_base; the code can just deal
> with guest physical addresses directly, as numbers, and leave the
> actual memory access to the existing physical memory abstraction.
>
> So I think it would be nice to have that change in qemu upstream.
> While it doesn't directly enable anything useful right away, the
> result is slightly cleaner.  I'll submit a proper patch shortly.
>   

Actually it is quite useful, with the 4GB+ support qemu memory is not 
linear since the pci hole is skipped in phys_ram_base.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Ian Jackson]

I wrote:
> In the attached patch, I remove all the direct uses of phys_ram_base
> from hw/pc.c, except for those presently needed to construct the
> arguments to the vga init functions.

Is there something wrong with my patch or the general approach ?
I haven't had any reply apart from this:

Avi Kivity writes ("Re: [Qemu-devel] phys_ram_base, direct access to
> guest memory"):
> Actually [these changes are] quite useful, with the 4GB+ support
> qemu memory is not linear since the pci hole is skipped in
> phys_ram_base.

Thanks,
Ian.

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Aurelien Jarno]

On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
> I wrote:
> > In the attached patch, I remove all the direct uses of phys_ram_base
> > from hw/pc.c, except for those presently needed to construct the
> > arguments to the vga init functions.
> 
> Is there something wrong with my patch or the general approach ?

It simply doesn't work. After applying it, I get:


qemu: fatal: Trying to execute code outside RAM or ROM at 0x000a0000

EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000400
ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000
EIP=000affea EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0
ES =0000 00000000 0000ffff 00000000
CS =f000 ffff0000 0000ffff 00000000
SS =0000 00000000 0000ffff 00000000
DS =0000 00000000 0000ffff 00000000
FS =0000 00000000 0000ffff 00000000
GS =0000 00000000 0000ffff 00000000
LDT=0000 00000000 0000ffff 00008000
TR =0000 00000000 0000ffff 00008000
GDT=     00000000 0000ffff
IDT=     00000000 0000ffff
CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000
CCS=00000000 CCD=00000000 CCO=ADDB    
FCW=037f FSW=0000 [ST=0] FTW=00 MXCSR=00001f80
FPR0=0000000000000000 0000 FPR1=0000000000000000 0000
FPR2=0000000000000000 0000 FPR3=0000000000000000 0000
FPR4=0000000000000000 0000 FPR5=0000000000000000 0000
FPR6=0000000000000000 0000 FPR7=0000000000000000 0000
XMM00=00000000000000000000000000000000 XMM01=00000000000000000000000000000000
XMM02=00000000000000000000000000000000 XMM03=00000000000000000000000000000000
XMM04=00000000000000000000000000000000 XMM05=00000000000000000000000000000000
XMM06=00000000000000000000000000000000 XMM07=00000000000000000000000000000000


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Blue Swirl]

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

On 4/8/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
>  > I wrote:
>  > > In the attached patch, I remove all the direct uses of phys_ram_base
>  > > from hw/pc.c, except for those presently needed to construct the
>  > > arguments to the vga init functions.
>  >
>  > Is there something wrong with my patch or the general approach ?
>
>
> It simply doesn't work. After applying it, I get:
>
>
>  qemu: fatal: Trying to execute code outside RAM or ROM at 0x000a0000

I fixed the bug in the patch, cpu_physical_memory_write_rom must be
used instead of cpu_physical_memory_write. I also made the same
changes to Sparc32/64, they run fine. Does this version work for PC
targets?

[-- Attachment #2: phys_mem_ij.diff --]
[-- Type: plain/text, Size: 20184 bytes --]

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Aurelien Jarno]

On Fri, May 02, 2008 at 05:52:07PM +0300, Blue Swirl wrote:
> On 4/8/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
> >  > I wrote:
> >  > > In the attached patch, I remove all the direct uses of phys_ram_base
> >  > > from hw/pc.c, except for those presently needed to construct the
> >  > > arguments to the vga init functions.
> >  >
> >  > Is there something wrong with my patch or the general approach ?
> >
> >
> > It simply doesn't work. After applying it, I get:
> >
> >
> >  qemu: fatal: Trying to execute code outside RAM or ROM at 0x000a0000
> 
> I fixed the bug in the patch, cpu_physical_memory_write_rom must be
> used instead of cpu_physical_memory_write. I also made the same
> changes to Sparc32/64, they run fine. Does this version work for PC
> targets?

Unfortunately the problem is still there, with the same error message.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Blue Swirl]

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

On 5/5/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Fri, May 02, 2008 at 05:52:07PM +0300, Blue Swirl wrote:
>  > On 4/8/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
>  > > On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
>  > >  > I wrote:
>  > >  > > In the attached patch, I remove all the direct uses of phys_ram_base
>  > >  > > from hw/pc.c, except for those presently needed to construct the
>  > >  > > arguments to the vga init functions.
>  > >  >
>  > >  > Is there something wrong with my patch or the general approach ?
>  > >
>  > >
>  > > It simply doesn't work. After applying it, I get:
>  > >
>  > >
>  > >  qemu: fatal: Trying to execute code outside RAM or ROM at 0x000a0000
>  >
>  > I fixed the bug in the patch, cpu_physical_memory_write_rom must be
>  > used instead of cpu_physical_memory_write. I also made the same
>  > changes to Sparc32/64, they run fine. Does this version work for PC
>  > targets?
>
>
> Unfortunately the problem is still there, with the same error message.

There were two additional problems, the offset was incorrect and the
memory was written before it was mapped. This version seems to work.

Any objections? May I commit this version?

[-- Attachment #2: phys_mem_pconly_ij.diff --]
[-- Type: plain/text, Size: 6733 bytes --]

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Fabrice Bellard]

Blue Swirl wrote:
> On 5/5/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> On Fri, May 02, 2008 at 05:52:07PM +0300, Blue Swirl wrote:
>>  > On 4/8/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
>>  > > On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
>>  > >  > I wrote:
>>  > >  > > In the attached patch, I remove all the direct uses of phys_ram_base
>>  > >  > > from hw/pc.c, except for those presently needed to construct the
>>  > >  > > arguments to the vga init functions.
>>  > >  >
>>  > >  > Is there something wrong with my patch or the general approach ?
>>  > >
>>  > >
>>  > > It simply doesn't work. After applying it, I get:
>>  > >
>>  > >
>>  > >  qemu: fatal: Trying to execute code outside RAM or ROM at 0x000a0000
>>  >
>>  > I fixed the bug in the patch, cpu_physical_memory_write_rom must be
>>  > used instead of cpu_physical_memory_write. I also made the same
>>  > changes to Sparc32/64, they run fine. Does this version work for PC
>>  > targets?
>>
>>
>> Unfortunately the problem is still there, with the same error message.
> 
> There were two additional problems, the offset was incorrect and the
> memory was written before it was mapped. This version seems to work.
> 
> Any objections? May I commit this version?

OK for the kernel loading, but not for the BIOS loading : there is no 
reason all the BIOS is mapped at a particular physical address (because 
this address is selectable by specific chipset bits), so it must really 
be loaded at ram addresses, not at physical addresses.

IMHO, it still makes sense to allow loading of data at a particular ram 
address.

Regards,

Fabrice.

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Blue Swirl]

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

On 5/13/08, Fabrice Bellard <fabrice@bellard.org> wrote:
> Blue Swirl wrote:
>
> > On 5/5/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >
> > > On Fri, May 02, 2008 at 05:52:07PM +0300, Blue Swirl wrote:
> > >  > On 4/8/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > >  > > On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
> > >  > >  > I wrote:
> > >  > >  > > In the attached patch, I remove all the direct uses of
> phys_ram_base
> > >  > >  > > from hw/pc.c, except for those presently needed to construct
> the
> > >  > >  > > arguments to the vga init functions.
> > >  > >  >
> > >  > >  > Is there something wrong with my patch or the general approach ?
> > >  > >
> > >  > >
> > >  > > It simply doesn't work. After applying it, I get:
> > >  > >
> > >  > >
> > >  > >  qemu: fatal: Trying to execute code outside RAM or ROM at
> 0x000a0000
> > >  >
> > >  > I fixed the bug in the patch, cpu_physical_memory_write_rom must be
> > >  > used instead of cpu_physical_memory_write. I also made the same
> > >  > changes to Sparc32/64, they run fine. Does this version work for PC
> > >  > targets?
> > >
> > >
> > > Unfortunately the problem is still there, with the same error message.
> > >
> >
> > There were two additional problems, the offset was incorrect and the
> > memory was written before it was mapped. This version seems to work.
> >
> > Any objections? May I commit this version?
> >
>
>  OK for the kernel loading, but not for the BIOS loading : there is no
> reason all the BIOS is mapped at a particular physical address (because this
> address is selectable by specific chipset bits), so it must really be loaded
> at ram addresses, not at physical addresses.
>
>  IMHO, it still makes sense to allow loading of data at a particular ram
> address.

I removed the BIOS loading parts. But is it possible to adjust VGA
BIOS and option ROM addresses by chipset? Their sizes are also limited
because wrong size files are rejected.

[-- Attachment #2: phys_mem_pconly_ij.diff --]
[-- Type: plain/text, Size: 3925 bytes --]

Re: [Qemu-devel] [PATCH] Re: phys_ram_base, direct access to guest memory

[Fabrice Bellard]

Blue Swirl wrote:
> On 5/13/08, Fabrice Bellard <fabrice@bellard.org> wrote:
>> Blue Swirl wrote:
>>
>>> On 5/5/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
>>>
>>>> On Fri, May 02, 2008 at 05:52:07PM +0300, Blue Swirl wrote:
>>>>  > On 4/8/08, Aurelien Jarno <aurelien@aurel32.net> wrote:
>>>>  > > On Tue, Mar 25, 2008 at 11:12:57AM +0000, Ian Jackson wrote:
>>>>  > >  > I wrote:
>>>>  > >  > > In the attached patch, I remove all the direct uses of
>> phys_ram_base
>>>>  > >  > > from hw/pc.c, except for those presently needed to construct
>> the
>>>>  > >  > > arguments to the vga init functions.
>>>>  > >  >
>>>>  > >  > Is there something wrong with my patch or the general approach ?
>>>>  > >
>>>>  > >
>>>>  > > It simply doesn't work. After applying it, I get:
>>>>  > >
>>>>  > >
>>>>  > >  qemu: fatal: Trying to execute code outside RAM or ROM at
>> 0x000a0000
>>>>  >
>>>>  > I fixed the bug in the patch, cpu_physical_memory_write_rom must be
>>>>  > used instead of cpu_physical_memory_write. I also made the same
>>>>  > changes to Sparc32/64, they run fine. Does this version work for PC
>>>>  > targets?
>>>>
>>>>
>>>> Unfortunately the problem is still there, with the same error message.
>>>>
>>> There were two additional problems, the offset was incorrect and the
>>> memory was written before it was mapped. This version seems to work.
>>>
>>> Any objections? May I commit this version?
>>>
>>  OK for the kernel loading, but not for the BIOS loading : there is no
>> reason all the BIOS is mapped at a particular physical address (because this
>> address is selectable by specific chipset bits), so it must really be loaded
>> at ram addresses, not at physical addresses.
>>
>>  IMHO, it still makes sense to allow loading of data at a particular ram
>> address.
> 
> I removed the BIOS loading parts. But is it possible to adjust VGA
> BIOS and option ROM addresses by chipset? 
> [...]

Yes. At least part of the BIOS can be replaced by RAM on a real PC (a
subset is implement is piix_pci.c).

Generically speaking, loading the content of a ROM/flash at a physical
address is almost always a bug or the indication that the corresponding
hardware model is greatly simplified.

There must be generic function to load data at a given ram address. If
you want to avoid the use of phys_ram_base, then you can add a specific
memcpy function similar to the one for the physical memory.

But it is not a good idea at all to suppress the access to the RAM memory !

Regards,

Fabrice.