Skip to contentSkip to conversation

add default pci subsystem id for all devices.

PATCH In review Gerd Hoffmann 2 participants 5 messages Aug 26, 2008 – Aug 27, 2008

lore
Share & tools
lore

Gerd Hoffmann

author
#0 lore raw

PATCH 2/2 add default pci subsystem id for all devices.

Hi,

This sets a default pci subsystem id for all emulated pci devices. I'm using fffa:0001 for now, we should probably get something assigned instead ...

cheers,
Gerd

Quoted 1 line

From 1acb8825f1537b55659498fb61d543ef5d015550 Mon Sep 17 00:00:00 2001

From: Gerd Hoffmann <[email protected]>
Date: Tue, 26 Aug 2008 12:34:42 +0200
Subject: [PATCH] pci: add default pci subsystem id for all devices.

This sets a default PCI subsystem ID for all emulated PCI devices. PCI specs require this, so do it. Individual devices can overwrite it of course. The defaults are global variables so they can easily be changed (before device creation) as Xen probably wants to use the XenSource vendor ID instead of the qemu default.

TODO: get an official vendor ID assigned, or borrow one (maybe
Qumranet which already sponsors the virtio IDs ???).

Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/pci.c |   11 +++++++++++
 hw/pci.h |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)
hw/pci.c +11 -0
index bc55989..ffc90d7 100644
@@ -50,6 +50,8 @@ static void pci_update_mappings(PCIDevice *d);
 static void pci_set_irq(void *opaque, int irq_num, int level);
 
 target_phys_addr_t pci_mem_base;
+uint16_t pci_default_sub_vendor_id = 0xfffa; /* FIXME: get one assigned */
+uint16_t pci_default_sub_device_id = 0x0001;
 static int pci_irq_index;
 static PCIBus *first_bus;
 

@@ -145,6 +147,14 @@ int pci_device_load(PCIDevice *s, QEMUFile *f)
     return 0;
 }
 
+static int pci_set_default_subsystem_id(PCIDevice *pci_dev)
+{
+    struct pci_config_header *conf = (void*)pci_dev->config;
+
+    conf->sub_vendor_id = cpu_to_le16(pci_default_sub_vendor_id);
+    conf->sub_device_id = cpu_to_le16(pci_default_sub_device_id);
+}
+
 /* -1 for devfn means auto assign */
 PCIDevice *pci_register_device(PCIBus *bus, const char *name,
                                int instance_size, int devfn,

@@ -171,6 +181,7 @@ PCIDevice *pci_register_device(PCIBus *bus, const char *name,
     pci_dev->devfn = devfn;
     pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
     memset(pci_dev->irq_state, 0, sizeof(pci_dev->irq_state));
+    pci_set_default_subsystem_id(pci_dev);
 
     if (!config_read)
         config_read = pci_default_read_config;
hw/pci.h +2 -0
index 2b1ebba..6e0d7ed 100644
@@ -33,6 +33,8 @@ struct pci_config_header {
 };
 
 extern target_phys_addr_t pci_mem_base;
+extern uint16_t pci_default_sub_vendor_id;
+extern uint16_t pci_default_sub_device_id;
 
 typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
                                 uint32_t address, uint32_t data, int len);
Signature · 1 line
1.5.5.1
#1 lore raw
Quoted 4 lines · Gerd Hoffmann

This sets a default PCI subsystem ID for all emulated PCI devices. PCI specs require this, so do it.  Individual devices can overwrite it of course.  The defaults are global variables so they can easily be changed (before device creation)

This is just asking for trouble. Having devices set magic global variables before they create the device is a truly horrible API.

Paul

author
#2 lore raw
Quoted 8 lines · Paul Brook
Quoted 4 lines · Gerd Hoffmann

This sets a default PCI subsystem ID for all emulated PCI devices. PCI specs require this, so do it. Individual devices can overwrite it of course. The defaults are global variables so they can easily be changed (before device creation)

This is just asking for trouble. Having devices set magic global variables before they create the device is a truly horrible API.

The global variables are just for the *default* value, i.e. what devices get prefilled by pci_register_device(). If a pci device wants set specific subsystem IDs it can just do so in the init function where the other config space fields are filled. There is no need for devices to ever fiddle with the global variables.

cheers,
Gerd

#3 lore raw
Quoted 7 lines · Gerd Hoffmann
Quoted 6 lines · Paul Brook
Quoted 4 lines · Gerd Hoffmann

This sets a default PCI subsystem ID for all emulated PCI devices. PCI specs require this, so do it. Individual devices can overwrite it of course. The defaults are global variables so they can easily be changed (before device creation)

Quoted 8 lines
Quoted 2 lines

This is just asking for trouble. Having devices set magic global variables before they create the device is a truly horrible API.

The global variables are just for the *default* value, i.e. what devices get prefilled by pci_register_device(). If a pci device wants set specific subsystem IDs it can just do so in the init function where the other config space fields are filled. There is no need for devices to ever fiddle with the global variables.

That's not what your comment says. Why are they extern variables if other modules aren't supposed to modify them?

Paul

author
#4 lore raw
Quoted 9 lines · Paul Brook
Quoted 8 lines · Gerd Hoffmann
Quoted 7 lines · Paul Brook
Quoted 4 lines · Gerd Hoffmann

This sets a default PCI subsystem ID for all emulated PCI devices. PCI specs require this, so do it. Individual devices can overwrite it of course. The defaults are global variables so they can easily be changed (before device creation)

This is just asking for trouble. Having devices set magic global variables before they create the device is a truly horrible API.

Quoted 8 lines
Quoted 5 lines

The global variables are just for the *default* value, i.e. what devices get prefilled by pci_register_device(). If a pci device wants set specific subsystem IDs it can just do so in the init function where the other config space fields are filled. There is no need for devices to ever fiddle with the global variables.

That's not what your comment says. Why are they extern variables if other modules aren't supposed to modify them?

The wording in the resent patch should be more clear.

The intention is to be able to change the default subsystem id based on the machine type. pci drivers can easily set that directly, they don't need this.

cheers,
Gerd