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

[J. Mayer]

On Fri, 2007-11-02 at 22:33 +0200, Blue Swirl wrote:
> 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.

The higher level routine need the low-level ones in the PowerPC case, as
they can be used on any kind of NVRAM device (or RAM, flash, ROM, ...).
Then I cannot have this high-levle routine without some specific helpers
at a lower level.
Accessing directly to the destination location would lead to hardcode
the physical hardware destination, and may not be possible if we want to
write something that might be (re)located, or need to be enabled before
usage, like a PCI device or the PowerPC 405 internal SRAM (which is not
visible in the physical address space at reset time).
Then, at a level or another, I need some low-level devices accessors.
For me the question is: is it useful to make those routines generic,
usable by any target to access any device, or is it to be a PowerPC
specific hack ? In the later case, I just have to remove the functions
prototypes from vl.h and not change your code to make the Sparc64 target
compile again.

-- 
J. Mayer <address@hidden>
Never organized