Re: [Qemu-devel] [PATCH 7/7] PPC: Qdev'ify e500 pci

2011-05-09 Thread Paul Brook 
 The e500 PCI controller isn't qdev'ified yet. This leads to severe issues
 when running with -drive.
 
 To be able to use a virtio disk with an e500 VM, let's convert the PCI
 controller over to qdev.
 
 Signed-off-by: Alexander Graf ag...@suse.de
Reviewed-by: Paul Brook p...@codesourcery.com

Re: [Qemu-devel] [PATCH 7/7] PPC: Qdev'ify e500 pci

2011-05-07 Thread Paul Brook 
 @@ -250,7 +254,6 @@ static const VMStateDescription vmstate_ppce500_pci = {
 
  .minimum_version_id = 1,
  .minimum_version_id_old = 1,
  .fields  = (VMStateField[]) {
 
 -VMSTATE_PCI_DEVICE_POINTER(pci_dev, PPCE500PCIState),

Doesn't this require incrementing version_id?

 +vmstate_register(dev-qdev, ppce500_pci_id++, vmstate_ppce500_pci,

ppce500_pci_id is bogus, and should be removed.
You probably shouldn't be calling this at all.  Instead use 
sysbus_register_withprop and qdev.vmsd.

Other than that, this patch looks ok.

Paul

Re: [Qemu-devel] [PATCH 7/7] PPC: Qdev'ify e500 pci

2011-05-07 Thread Alexander Graf 

On 08.05.2011, at 01:48, Paul Brook wrote:

 @@ -250,7 +254,6 @@ static const VMStateDescription vmstate_ppce500_pci = {
 
 .minimum_version_id = 1,
 .minimum_version_id_old = 1,
 .fields  = (VMStateField[]) {
 
 -VMSTATE_PCI_DEVICE_POINTER(pci_dev, PPCE500PCIState),
 
 Doesn't this require incrementing version_id?

It's never worked before and there is no known good state - so I don't think 
there's any need to increment here whatsoever :). There won't be any old users.

 
 +vmstate_register(dev-qdev, ppce500_pci_id++, vmstate_ppce500_pci,
 
 ppce500_pci_id is bogus, and should be removed.
 You probably shouldn't be calling this at all.  Instead use 
 sysbus_register_withprop and qdev.vmsd.
 
 Other than that, this patch looks ok.

Alrighty, new patch underway.


Alex

Re: [Qemu-devel] [PATCH 7/7] PPC: Qdev'ify e500 pci

2011-05-06 Thread Paul Brook 
 PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t
 registers) {
 -PPCE500PCIState *controller;
 +DeviceState *dev;
 +PCIBus *b;
 +PCIHostState *h;
 +PPCE500PCIState *s;
  PCIDevice *d;
 -int index;
  static int ppce500_pci_id;
 +SysBusDevice *sb;
 +
 +dev = qdev_create(NULL, e500-pcihost);
 +sb = sysbus_from_qdev(dev);
 +h = FROM_SYSBUS(PCIHostState, sb);
 +s = DO_UPCAST(PPCE500PCIState, pci_state, h);
 +
 +b = pci_register_bus(s-pci_state.busdev.qdev, NULL,

No.  This function should not exist.  All this should be done in 
e500_pcihost_initfn.  Please do the qdev conversion properly.

See versatilepb.c/versatile_pci.c for an example of how this is supposed to be 
done.

 +.qdev.no_user = 1,

There's no good reason for this. It indicates you did the qdev conversion 
incorrectly.

Paul

Re: [Qemu-devel] [PATCH 7/7] PPC: Qdev'ify e500 pci

2011-05-06 Thread Paul Brook 
  PCIBus *ppce500_pci_init(qemu_irq pci_irqs[4], target_phys_addr_t
  registers) {
  -PPCE500PCIState *controller;
  +DeviceState *dev;
  +PCIBus *b;
  +PCIHostState *h;
  +PPCE500PCIState *s;
  
   PCIDevice *d;
  
  -int index;
  
   static int ppce500_pci_id;
  
  +SysBusDevice *sb;
  +
  +dev = qdev_create(NULL, e500-pcihost);
  +sb = sysbus_from_qdev(dev);
  +h = FROM_SYSBUS(PCIHostState, sb);
  +s = DO_UPCAST(PPCE500PCIState, pci_state, h);
  +
  +b = pci_register_bus(s-pci_state.busdev.qdev, NULL,
 
 No.  This function should not exist.  All this should be done in
 e500_pcihost_initfn.  Please do the qdev conversion properly.

Or more precicely it should not depend on the internals of ppce500_pci.c.
In principle the only public entry point in that file should be 
device_init(...).  If used by multiple boards a simple helper function may be 
appropriate (e.g. smc91c111_init).  Note that this helper function has no ties 
to the rest of that file, and could be trivially moved into a different file 
or replaced with a macro/inline funciton.

ppce500_pci_init definitely should not be creating the PCI bus.

Paul