RE: [Qemu-devel] [PATCH 4/5] kvm/powerpc: Add MPC8544DS board support

[Liu Yu-B13201]

> -----Original Message-----
> From: Blue Swirl [mailto:address@hidden 
> Sent: Tuesday, February 17, 2009 11:28 PM
> To: address@hidden
> Cc: address@hidden; address@hidden; Liu Yu-B13201; 
> address@hidden
> Subject: Re: [Qemu-devel] [PATCH 4/5] kvm/powerpc: Add 
> MPC8544DS board support
> 
> On 2/17/09, Liu Yu <address@hidden> wrote:
> > This patch add emulation of MPC8544DS board.
> >  And it can work on All E500 platforms.
> >
> >  Signed-off-by: Liu Yu <address@hidden>
> 
> >  + * Copyright (C) 2009 Freescale Semiconductor, Inc. All 
> rights reserved.
> 
> Again, "All rights reserved" is not what GPL says.
> 
> >  +    ret = kvmppc_read_host_property(node, prop, &cell, 
> sizeof(cell));
> >  +    if (ret < 0) {
> >  +        fprintf(stderr, "couldn't read host %s/%s\n", node, prop);
> >  +        goto out;
> >  +    }
> 
> For the Qemu case, the host CPU or system may be different from the
> emulated target CPU or system. These should be moved to
> target-ppc/kvm_ppc.c.
> 

kvmppc_read_host_property() is already in target-ppc/kvm_ppc.c.
Move these to target-ppc/kvm_ppc.c then still need a interface to copy the cell 
value out.
And the these code is only called under the condition kvm_enabled().

> >  +    path = qemu_malloc(pathlen);
> >  +    if (path == NULL)
> >  +        goto out;
> 
> qemu_malloc will not return NULL anymore, please remove the check.
> 

Fixed.

> >  +    free(path);
> 
> s/free/qemu_free/g
> 
> >  +                sprintf(buf, "/cpus/%s", dirp->d_name);
> 
> Please use snprintf.
> 

Fixed.

> >  +    /* Register Memory */
> >  +    cpu_register_physical_memory(0, ram_size, 0);
> 
> This was the old way. Please use qemu_ram_alloc instead.
> 

Does it work for kvm case?
I failed when use qemu_ram_alloc.