[Qemu-devel] [patch 2/2] add default pci subsystem id for all devices.

[Qemu-devel] [patch 2/2] add default pci subsystem id for all devices.

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 173 bytes --]

  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

[-- Attachment #2: 0019-pci-add-default-pci-subsystem-id-for-all-devices.patch --]
[-- Type: text/plain, Size: 2539 bytes --]

>From 1acb8825f1537b55659498fb61d543ef5d015550 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
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 <kraxel@redhat.com>
---
 hw/pci.c |   11 +++++++++++
 hw/pci.h |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index bc55989..ffc90d7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -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;
diff --git a/hw/pci.h b/hw/pci.h
index 2b1ebba..6e0d7ed 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -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);
-- 
1.5.5.1

Re: [Qemu-devel] [patch 2/2] add default pci subsystem id for all devices.

[Paul Brook]

On Tuesday 26 August 2008, Gerd Hoffmann wrote:
> 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

Re: [Qemu-devel] [patch 2/2] add default pci subsystem id for all devices.

[Gerd Hoffmann]

Paul Brook wrote:
> On Tuesday 26 August 2008, Gerd Hoffmann wrote:
>> 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

Re: [Qemu-devel] [patch 2/2] add default pci subsystem id for all devices.

[Paul Brook]

On Wednesday 27 August 2008, Gerd Hoffmann wrote:
> Paul Brook wrote:
> > On Tuesday 26 August 2008, Gerd Hoffmann wrote:
> >> 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.

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

Paul

Re: [Qemu-devel] [patch 2/2] add default pci subsystem id for all devices.

[Gerd Hoffmann]

Paul Brook wrote:
> On Wednesday 27 August 2008, Gerd Hoffmann wrote:
>> Paul Brook wrote:
>>> On Tuesday 26 August 2008, Gerd Hoffmann wrote:
>>>> 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.
> 
> 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