[Qemu-devel] [patch 0/2] RFC: move PCI device enumeration out of pci drivers

[Qemu-devel] [patch 0/2] RFC: move PCI device enumeration out of pci drivers

[Marcelo Tosatti]

$SUBJECT. This should make it easier for when PCI device addresses are specified
somewhere else.

Based on Markus patch to specify PCI dev address on command line (pci= parameter).

Let me know if you guys are OK with this for a patch covering all
pci_register_device users.

[Qemu-devel] [patch 1/2] qemu: move pci devfn "first free" assignment to separate function

[Marcelo Tosatti]

[-- Attachment #1: pci-assign-dev-addr --]
[-- Type: text/plain, Size: 1550 bytes --]

Move pci device address assignment to a separate function.

Index: trunk/hw/pci.c
===================================================================
--- trunk.orig/hw/pci.c
+++ trunk/hw/pci.c
@@ -121,6 +121,16 @@ int pci_bus_num(PCIBus *s)
     return s->bus_num;
 }
 
+int pci_bus_assign_dev_addr(PCIBus *bus)
+{
+    int devfn;
+
+    for(devfn = bus->devfn_min ; devfn < 256; devfn += 8)
+        if (!bus->devices[devfn])
+                return devfn;
+    return -1;
+}
+
 void pci_device_save(PCIDevice *s, QEMUFile *f)
 {
     int i;
@@ -246,14 +256,6 @@ PCIDevice *pci_register_device(PCIBus *b
     if (pci_irq_index >= PCI_DEVICES_MAX)
         return NULL;
 
-    if (devfn < 0) {
-        for(devfn = bus->devfn_min ; devfn < 256; devfn += 8) {
-            if (!bus->devices[devfn])
-                goto found;
-        }
-        return NULL;
-    found: ;
-    }
     pci_dev = qemu_mallocz(instance_size);
     pci_dev->bus = bus;
     pci_dev->devfn = devfn;
Index: trunk/hw/pci.h
===================================================================
--- trunk.orig/hw/pci.h
+++ trunk/hw/pci.h
@@ -184,6 +184,7 @@ PCIDevice *pci_nic_init(PCIBus *bus, NIC
 void pci_data_write(void *opaque, uint32_t addr, uint32_t val, int len);
 uint32_t pci_data_read(void *opaque, uint32_t addr, int len);
 int pci_bus_num(PCIBus *s);
+int pci_bus_assign_dev_addr(PCIBus *bus);
 void pci_for_each_device(int bus_num, void (*fn)(PCIDevice *d));
 PCIBus *pci_find_bus(int bus_num);
 PCIDevice *pci_find_device(int bus_num, int slot, int function);

Re: [Qemu-devel] [patch 1/2] qemu: move pci devfn "first free" assignment to separate function

[Avi Kivity]

Marcelo Tosatti wrote:
> Move pci device address assignment to a separate function.
>
> Index: trunk/hw/pci.c
> ===================================================================
> --- trunk.orig/hw/pci.c
> +++ trunk/hw/pci.c
> @@ -121,6 +121,16 @@ int pci_bus_num(PCIBus *s)
>      return s->bus_num;
>  }
>  
> +int pci_bus_assign_dev_addr(PCIBus *bus)
> +{
> +    int devfn;
> +
> +    for(devfn = bus->devfn_min ; devfn < 256; devfn += 8)
> +        if (!bus->devices[devfn])
> +                return devfn;
> +    return -1;
> +}
>   

s/assign/allocate/ or get_free, to avoid confusion with device 
assignment (and given that you aren't assigning anything).


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

[Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Marcelo Tosatti]

[-- Attachment #1: pci-refactor-devfn --]
[-- Type: text/plain, Size: 7403 bytes --]

Some pci device initialization functions do not accept a devfn parameter,
but instead use "-1", which caused pci_register_device to find the first
free slot on the given bus.

Have them accept a "devfn" parameter, and use the newly introduced
pci_bus_assign_dev_addr function on platform init code to perform
the "first free" enumeration.

Some pci init functions still hardcode devfn (usually the host bridge of the bus
with devfn 0), those will have be to changed later.

 
Index: trunk/hw/cirrus_vga.c
===================================================================
--- trunk.orig/hw/cirrus_vga.c
+++ trunk/hw/cirrus_vga.c
@@ -3358,7 +3358,7 @@ static void pci_cirrus_write_config(PCID
     vga_dirty_log_start((VGAState *)s);
 }
 
-void pci_cirrus_vga_init(PCIBus *bus, uint8_t *vga_ram_base,
+void pci_cirrus_vga_init(PCIBus *bus, int devfn, uint8_t *vga_ram_base,
                          ram_addr_t vga_ram_offset, int vga_ram_size)
 {
     PCICirrusVGAState *d;
@@ -3371,7 +3371,8 @@ void pci_cirrus_vga_init(PCIBus *bus, ui
     /* setup PCI configuration registers */
     d = (PCICirrusVGAState *)pci_register_device(bus, "Cirrus VGA",
                                                  sizeof(PCICirrusVGAState),
-                                                 -1, NULL, pci_cirrus_write_config);
+                                                 devfn, NULL,
+                                                 pci_cirrus_write_config);
     pci_conf = d->dev.config;
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CIRRUS);
     pci_config_set_device_id(pci_conf, device_id);
Index: trunk/hw/pc.c
===================================================================
--- trunk.orig/hw/pc.c
+++ trunk/hw/pc.c
@@ -946,7 +946,7 @@ vga_bios_error:
 
     if (pci_enabled) {
         pci_bus = i440fx_init(&i440fx_state, i8259);
-        piix3_devfn = piix3_init(pci_bus, -1);
+        piix3_devfn = piix3_init(pci_bus, pci_bus_assign_dev_addr(pci_bus));
     } else {
         pci_bus = NULL;
     }
@@ -959,6 +959,7 @@ vga_bios_error:
     if (cirrus_vga_enabled) {
         if (pci_enabled) {
             pci_cirrus_vga_init(pci_bus,
+                                pci_bus_assign_dev_addr(pci_bus),
                                 phys_ram_base + vga_ram_addr,
                                 vga_ram_addr, vga_ram_size);
         } else {
@@ -967,13 +968,15 @@ vga_bios_error:
         }
     } else if (vmsvga_enabled) {
         if (pci_enabled)
-            pci_vmsvga_init(pci_bus, phys_ram_base + vga_ram_addr,
+            pci_vmsvga_init(pci_bus, pci_bus_assign_dev_addr(pci_bus),
+                            phys_ram_base + vga_ram_addr,
                             vga_ram_addr, vga_ram_size);
         else
             fprintf(stderr, "%s: vmware_vga: no PCI bus\n", __FUNCTION__);
     } else if (std_vga_enabled) {
         if (pci_enabled) {
-            pci_vga_init(pci_bus, phys_ram_base + vga_ram_addr,
+            pci_vga_init(pci_bus, pci_bus_assign_dev_addr(pci_bus),
+                         phys_ram_base + vga_ram_addr,
                          vga_ram_addr, vga_ram_size, 0, 0);
         } else {
             isa_vga_init(phys_ram_base + vga_ram_addr,
@@ -1020,7 +1023,8 @@ vga_bios_error:
         if (!pci_enabled || (nd->model && strcmp(nd->model, "ne2k_isa") == 0))
             pc_init_ne2k_isa(nd, i8259);
         else
-            pci_nic_init(pci_bus, nd, -1, "ne2k_pci");
+            pci_nic_init(pci_bus, nd, pci_bus_assign_dev_addr(pci_bus),
+                         "ne2k_pci");
     }
 
     qemu_system_hot_add_init();
@@ -1091,7 +1095,7 @@ vga_bios_error:
         max_bus = drive_get_max_bus(IF_SCSI);
 
 	for (bus = 0; bus <= max_bus; bus++) {
-            scsi = lsi_scsi_init(pci_bus, -1);
+            scsi = lsi_scsi_init(pci_bus, pci_bus_assign_dev_addr(pci_bus));
             for (unit = 0; unit < LSI_MAX_DEVS; unit++) {
 	        index = drive_get_index(IF_SCSI, bus, unit);
 		if (index == -1)
Index: trunk/hw/pc.h
===================================================================
--- trunk.orig/hw/pc.h
+++ trunk/hw/pc.h
@@ -146,7 +146,7 @@ extern enum vga_retrace_method vga_retra
 
 int isa_vga_init(uint8_t *vga_ram_base,
                  unsigned long vga_ram_offset, int vga_ram_size);
-int pci_vga_init(PCIBus *bus, uint8_t *vga_ram_base,
+int pci_vga_init(PCIBus *bus, int devfn, uint8_t *vga_ram_base,
                  unsigned long vga_ram_offset, int vga_ram_size,
                  unsigned long vga_bios_offset, int vga_bios_size);
 int isa_vga_mm_init(uint8_t *vga_ram_base,
@@ -155,7 +155,7 @@ int isa_vga_mm_init(uint8_t *vga_ram_bas
                     int it_shift);
 
 /* cirrus_vga.c */
-void pci_cirrus_vga_init(PCIBus *bus, uint8_t *vga_ram_base,
+void pci_cirrus_vga_init(PCIBus *bus, int devfn, uint8_t *vga_ram_base,
                          ram_addr_t vga_ram_offset, int vga_ram_size);
 void isa_cirrus_vga_init(uint8_t *vga_ram_base,
                          ram_addr_t vga_ram_offset, int vga_ram_size);
Index: trunk/hw/pci.h
===================================================================
--- trunk.orig/hw/pci.h
+++ trunk/hw/pci.h
@@ -220,7 +220,7 @@ void lsi_scsi_attach(void *opaque, Block
 void *lsi_scsi_init(PCIBus *bus, int devfn);
 
 /* vmware_vga.c */
-void pci_vmsvga_init(PCIBus *bus, uint8_t *vga_ram_base,
+void pci_vmsvga_init(PCIBus *bus, int devfn, uint8_t *vga_ram_base,
                      unsigned long vga_ram_offset, int vga_ram_size);
 
 /* usb-uhci.c */
Index: trunk/hw/vga.c
===================================================================
--- trunk.orig/hw/vga.c
+++ trunk/hw/vga.c
@@ -2500,7 +2500,7 @@ static void pci_vga_write_config(PCIDevi
     vga_dirty_log_start(s);
 }
 
-int pci_vga_init(PCIBus *bus, uint8_t *vga_ram_base,
+int pci_vga_init(PCIBus *bus, int devfn, uint8_t *vga_ram_base,
                  unsigned long vga_ram_offset, int vga_ram_size,
                  unsigned long vga_bios_offset, int vga_bios_size)
 {
@@ -2510,7 +2510,7 @@ int pci_vga_init(PCIBus *bus, uint8_t *v
 
     d = (PCIVGAState *)pci_register_device(bus, "VGA",
                                            sizeof(PCIVGAState),
-                                           -1, NULL, pci_vga_write_config);
+                                           devfn, NULL, pci_vga_write_config);
     if (!d)
         return -1;
     s = &d->vga_state;
Index: trunk/hw/vmware_vga.c
===================================================================
--- trunk.orig/hw/vmware_vga.c
+++ trunk/hw/vmware_vga.c
@@ -1215,7 +1215,7 @@ static void pci_vmsvga_map_mem(PCIDevice
 
 #define PCI_CLASS_HEADERTYPE_00h	0x00
 
-void pci_vmsvga_init(PCIBus *bus, uint8_t *vga_ram_base,
+void pci_vmsvga_init(PCIBus *bus, int devfn, uint8_t *vga_ram_base,
                      unsigned long vga_ram_offset, int vga_ram_size)
 {
     struct pci_vmsvga_state_s *s;
@@ -1223,7 +1223,7 @@ void pci_vmsvga_init(PCIBus *bus, uint8_
     /* Setup PCI configuration */
     s = (struct pci_vmsvga_state_s *)
         pci_register_device(bus, "QEMUware SVGA",
-                sizeof(struct pci_vmsvga_state_s), -1, 0, 0);
+                sizeof(struct pci_vmsvga_state_s), devfn, 0, 0);
     pci_config_set_vendor_id(s->card.config, PCI_VENDOR_ID_VMWARE);
     pci_config_set_device_id(s->card.config, SVGA_PCI_DEVICE_ID);
     s->card.config[PCI_COMMAND]		= 0x07;		/* I/O + Memory */

Re: [Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Paul Brook]

On Monday 13 April 2009, Marcelo Tosatti wrote:
> Some pci device initialization functions do not accept a devfn parameter,
> but instead use "-1", which caused pci_register_device to find the first
> free slot on the given bus.
>
> Have them accept a "devfn" parameter, and use the newly introduced
> pci_bus_assign_dev_addr function on platform init code to perform
> the "first free" enumeration.

I don't see how this is better. If anything we want the platform code to get 
smaller, not larger.

Paul

Re: [Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Marcelo Tosatti]

On Mon, Apr 13, 2009 at 01:27:34PM +0100, Paul Brook wrote:
> On Monday 13 April 2009, Marcelo Tosatti wrote:
> > Some pci device initialization functions do not accept a devfn parameter,
> > but instead use "-1", which caused pci_register_device to find the first
> > free slot on the given bus.
> >
> > Have them accept a "devfn" parameter, and use the newly introduced
> > pci_bus_assign_dev_addr function on platform init code to perform
> > the "first free" enumeration.
> 
> I don't see how this is better. If anything we want the platform code to get 
> smaller, not larger.

Paul,

This is a intermediate step. Later you'd get rid of
pci_bus_assign_dev_addr in platform code.

Ideally you'd have (I think):

- generic machine initialization code fills in details of pci device
  address in pci device structure (or a temporary placeholder) taken from
  machine description (optionally auto assigns from a free slot).

- pci driver init functions take generic device pointer as an
  argument (either containing the pre allocated pci device or temporary
  placeholder), and pass that to pci_register_device, which does
  actual registration of pci device on particular bus.

Does that make sense?

A benefit of the proposed patch is that it moves assumptions about pci  
device address assignment from drivers up to platform code. Later you'd
remove that from platform code to devtree data.

Re: [Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Brian Wheeler]

On Mon, 2009-04-13 at 10:30 -0300, Marcelo Tosatti wrote:
> On Mon, Apr 13, 2009 at 01:27:34PM +0100, Paul Brook wrote:
> > On Monday 13 April 2009, Marcelo Tosatti wrote:
> > > Some pci device initialization functions do not accept a devfn parameter,
> > > but instead use "-1", which caused pci_register_device to find the first
> > > free slot on the given bus.
> > >
> > > Have them accept a "devfn" parameter, and use the newly introduced
> > > pci_bus_assign_dev_addr function on platform init code to perform
> > > the "first free" enumeration.
> > 
> > I don't see how this is better. If anything we want the platform code to get 
> > smaller, not larger.
> 
> Paul,
> 
> This is a intermediate step. Later you'd get rid of
> pci_bus_assign_dev_addr in platform code.
> 
> Ideally you'd have (I think):
> 
> - generic machine initialization code fills in details of pci device
>   address in pci device structure (or a temporary placeholder) taken from
>   machine description (optionally auto assigns from a free slot).

> - pci driver init functions take generic device pointer as an
>   argument (either containing the pre allocated pci device or temporary
>   placeholder), and pass that to pci_register_device, which does
>   actual registration of pci device on particular bus.
> 
> Does that make sense?
> 
> A benefit of the proposed patch is that it moves assumptions about pci  
> device address assignment from drivers up to platform code. Later you'd
> remove that from platform code to devtree data.
> 

Does this mean that there would be a mechanism for a platform to map the
pci memory address to a platform specific linear address?  Alpha uses 

0x8000000000 + (0x20000000 * bus) + address

to determine where the pci memory is mapped in the cpu address space...


Brian

Re: [Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Marcelo Tosatti]

On Mon, Apr 13, 2009 at 09:47:39AM -0400, Brian Wheeler wrote:
> Does this mean that there would be a mechanism for a platform to map the
> pci memory address to a platform specific linear address?  Alpha uses 
> 
> 0x8000000000 + (0x20000000 * bus) + address
> 
> to determine where the pci memory is mapped in the cpu address space...

I think so yes. The use case in mind for us is interrupt mapping from
(device address + PCI interrupt specifier) -> platform interrupt
specifier.

Re: [Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Marcelo Tosatti]

On Mon, Apr 13, 2009 at 11:34:46AM -0300, Marcelo Tosatti wrote:
> On Mon, Apr 13, 2009 at 09:47:39AM -0400, Brian Wheeler wrote:
> > Does this mean that there would be a mechanism for a platform to map the
> > pci memory address to a platform specific linear address?  Alpha uses 
> > 
> > 0x8000000000 + (0x20000000 * bus) + address
> > 
> > to determine where the pci memory is mapped in the cpu address space...
> 
> I think so yes. 

Actually, no.

> The use case in mind for us is interrupt mapping from
> (device address + PCI interrupt specifier) -> platform interrupt
> specifier.

Note: the discussion here is about _pci device address_:
bus,device,function.

Re: [Qemu-devel] [patch 2/2] qemu: switch pci device init functions to accept devfn

[Marcelo Tosatti]

On Mon, Apr 13, 2009 at 01:27:34PM +0100, Paul Brook wrote:
> On Monday 13 April 2009, Marcelo Tosatti wrote:
> > Some pci device initialization functions do not accept a devfn parameter,
> > but instead use "-1", which caused pci_register_device to find the first
> > free slot on the given bus.
> >
> > Have them accept a "devfn" parameter, and use the newly introduced
> > pci_bus_assign_dev_addr function on platform init code to perform
> > the "first free" enumeration.
> 
> I don't see how this is better. If anything we want the platform code to get 
> smaller, not larger.

OK, so, what would you like to see? The proposal where pci dev address
goes directly from devtree to pci_register_device, behind an opaque 
type in the way? Or goes through QEMUDevice? 

Please be more specific, so we can get somewhere.