Re: [Qemu-devel] qemu Makefile.target vl.h hw/cuda.c hw/grackle_...

[Blue Swirl]

On 11/2/07, Jocelyn Mayer <address@hidden> wrote:
>
> On Fri, 2007-11-02 at 17:18 +0200, Blue Swirl wrote:
> > On 11/2/07, J. Mayer <address@hidden> wrote:
> > >
> > > On Thu, 2007-11-01 at 23:13 +0100, J. Mayer wrote:
> > > > On Thu, 2007-11-01 at 21:53 +0200, Blue Swirl wrote:
> > > > > On 11/1/07, Blue Swirl <address@hidden> wrote:
> > > > > > On 10/29/07, Jocelyn Mayer <address@hidden> wrote:
> > > > > > > CVSROOT:        /sources/qemu
> > > > > > > Module name:    qemu
> > > > > > > Changes by:     Jocelyn Mayer <j_mayer> 07/10/28 23:42:18
> > > > > > >
> > > > > > > Modified files:
> > > > > > >         .              : Makefile.target vl.h
> > > > > > >         hw             : cuda.c grackle_pci.c heathrow_pic.c ppc.c
> > > > > > >                          ppc_chrp.c ppc_prep.c
> > > > > > > Added files:
> > > > > > >         hw             : mac_dbdma.c mac_nvram.c macio.c ppc_mac.h
> > > > > > >                          ppc_oldworld.c
> > > > > > >
> > > > > > > Log message:
> > > > > > >         * sort the PowerPC target object files
> > > > > > >         * make PowerPC NVRAM accessors generic to be able to use 
> > > > > > > a MacIO NVRAM
> > > > > > >           instead of the M48T59 one
> > > > > > >         * split PowerMac targets code:
> > > > > > >          - move all PowerMac related definitions and prototypes 
> > > > > > > into hw/ppc_mac.h
> > > > > > >          - add hw/mac_dbdma.c, hw/mac_nvram.c and macio.c
> > > > > > >            which implements shared PowerMac devices
> > > > > > >          - define the g3bw machine in a new hw/ppc_oldworld.c file
> > > > > > >         * Fix the g3bw target:
> > > > > > >          - fix the Grackle host PCI device
> > > > > > >          - connect the Heathrow PIC to the PowerPC 6xx bus pins
> > > > > > >
> > > > > > > CVSWeb URLs:
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/Makefile.target?cvsroot=qemu&r1=1.212&r2=1.213
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/vl.h?cvsroot=qemu&r1=1.280&r2=1.281
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/cuda.c?cvsroot=qemu&r1=1.16&r2=1.17
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/grackle_pci.c?cvsroot=qemu&r1=1.6&r2=1.7
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/heathrow_pic.c?cvsroot=qemu&r1=1.5&r2=1.6
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc.c?cvsroot=qemu&r1=1.34&r2=1.35
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_chrp.c?cvsroot=qemu&r1=1.44&r2=1.45
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_prep.c?cvsroot=qemu&r1=1.47&r2=1.48
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mac_dbdma.c?cvsroot=qemu&rev=1.1
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/mac_nvram.c?cvsroot=qemu&rev=1.1
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/macio.c?cvsroot=qemu&rev=1.1
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_mac.h?cvsroot=qemu&rev=1.1
> > > > > > > http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_oldworld.c?cvsroot=qemu&rev=1.1
> > > > > >
> > > > > > You broke sparc64-softmmu build with this patch.
> > > >
> > > > I am missing something ? I rebuilt all available targets before
> > > > commiting... but I now see sparc64-softmmu seems not to be in the
> > > > available targets, which could explain I cannot check if it compiles or
> > > > not...  As it been removed by mistake ?
> > > >
> > > > > I think the best solution to fix this is to put the nvram helpers to
> > > > > m48t59.h as inline functions  instead of duplicating the code in
> > > > > several places.
> > > >
> > > > You mean the NVRAM_set / get_xxx ? I was to remove the definitions from
> > > > vl.h, I have to say, because those are supposed to be PowerPC (in fact
> > > > OpenHack'Ware) related hacks.
> > > > Those functions will never go in m48t59.h as they are not related with
> > > > m48t59. Apple machine don't have such a device (even if Qemu pretend it
> > > > has, this is to be removed in the days to come) but need those functions
> > > > to pass arguments to the firmware. What I needed to do (and that what I
> > > > did commit) is make those routines independant from m48t59 so I can
> > > > remove this device from ppc_chrp.c and ppc_oldworld.c and use the real
> > > > Mac nvram instead (but ppc_prep.c still uses m48t59...).
> >
> > I see. Should sun4m use these functions too? On the other hand, there
> > is no need to be too independent on Sparc, because I think all Sun4u
> > machines use m48t59 and sun4m machines have either m48t08 or older
> > m48t02 (not supported yet). So if you prefer, sun4u could use the same
> > approach as sun4m and not use these functions?
>
> Depends on how you feel about it...
> If there is a real need to have a generic devices registers and/or
> internal memory accessor used during the target machine initialisation
> (the model I propose could be used not only for NVRAM...), then the code
> should be made more generic (ie renaming the nvram_t type with a more
> generic name) and only one implementation should be kept. If this is
> only useful for the PowerPC target initialisation, then you should keep
> using the m48t59 only implementation you have now for Sparc64 and the
> PowerPC NVRAM_xxx functions should not be declared in vl.h.
> I would say that having a generic accessor for devices during machine
> init can be useful and the implementation I provided may be sufficient
> for most usages. But it may also be not so useful ? ...

Well, I'd find common higher level routines more useful, like filling
OHW nvram structure v1 (used by Sun4m, Sun4u, PPC) or setting up
OpenBIOS variable partition using data from prom_env (Sun4m, Sun4u).
Or lower: write a 32 bit BE value to a location. Maybe these would be
more widely useful if they used generic access routines that are not
dependent on the type.