[Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 bswap.h      |    7 +++++++
 hw/usb-net.c |    2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/bswap.h b/bswap.h
index 523d805..b294d96 100644
--- a/bswap.h
+++ b/bswap.h
@@ -206,4 +206,11 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 #undef le_bswaps
 #undef be_bswaps
 
+typedef uint16_t le16;
+typedef uint32_t le32;
+typedef uint64_t le64;
+typedef uint16_t be16;
+typedef uint32_t be32;
+typedef uint64_t be64;
+
 #endif /* BSWAP_H */
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 82005af..569cd7d 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -326,8 +326,6 @@ enum {
 #define OID_PNP_REMOVE_WAKE_UP_PATTERN	0xfd010104
 #define OID_PNP_ENABLE_WAKE_UP		0xfd010106
 
-typedef uint32_t le32;
-
 typedef struct rndis_init_msg_type {
     le32 MessageType;
     le32 MessageLength;
-- 
1.5.5.1

[Qemu-devel] [PATCH 2/4] pci: add config space struct (from qemu-xen).

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/hw/pci.h b/hw/pci.h
index e870987..f518e5e 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -6,6 +6,32 @@
 
 /* PCI bus */
 
+struct pci_config_header {
+    le16    vendor_id;
+    le16    device_id;
+    le16    command;
+    le16    status;
+    uint8_t revision;
+    uint8_t api;
+    uint8_t subclass;
+    uint8_t class;
+    uint8_t cache_line_size; /* Units of 32 bit words */
+    uint8_t latency_timer;   /* In units of bus cycles */
+    uint8_t header_type;     /* Should be 0 */
+    uint8_t bist;            /* Built in self test */
+    le32    base_address_regs[6];
+    le32    reserved1;
+    le16    sub_vendor_id;
+    le16    sub_device_id;
+    le32    rom_addr;
+    le32    reserved3;
+    le32    reserved4;
+    uint8_t interrupt_line;
+    uint8_t interrupt_pin;
+    uint8_t min_gnt;
+    uint8_t max_lat;
+};
+
 extern target_phys_addr_t pci_mem_base;
 
 typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
-- 
1.5.5.1

[Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

This sets a default PCI subsystem ID for all emulated PCI devices.  PCI
specs require this, so do it.

Some background info:

Each PCI device has a PCI ID.  It should also have a PCI Subsystem ID.
The PCI subsystem ID wasn't mandatory in early PCI spec revisions, IIRC
2.0 added that requirement.  Thats why you usually get away without a
PCI subsystem ID, although it isn't correct.

Both IDs have a 16bit vendor and a 16bit device part.  The vendor IDs
are handed out by the PCI SIG.  The device IDs are assigned by the
vendor owning the vendor ID.

The PCI ID tells you which PCI chip is used, whereas the the PCI
Subsystem ID identifies the actual device (created using the PCI chip).

Example #1:  TV cards.  All TV cards using the bt878 chipset (first and
only one on the market for a few years in the 90-ies) have the same PCI
ID, the one of the bt878 chip.  But each TV card has different PCI
Subsystem IDs, which can be used to figure what the actual TV card is.

Example #2:  Laptops.  It is quite common to find the PCI Subsytem ID
pointing to the laptop vendor and model.  So the PCI ID says
'this is a intel ich7 ide controller', whereas the the PCI Subsystem ID
says 'this ich7 sits in a lenovo thinkpad', like this:

--------- cut here ----------
[root@zweiblum ~]# lspci -vs1f.1
00:1f.1 IDE interface: Intel Corporation 82801G (ICH7 Family) IDE
Controller (rev 02) (prog-if 8a [Master SecP PriP])
        Subsystem: Lenovo ThinkPad T60/R60 series
        [ ... more stuff snipped ... ]
--------- cut here ----------

In many cases it is enougth to know the PCI ID to handle a device
correctly.  Sometimes a device driver must identify the exact piece of
hardware (via PCI Subsystem ID) though.

What does this patch to qemu devices:

Right now the emulated PCI devices have no PCI subsystem ID, only the
PCI ID.  The discussed patch sets a default PCI subsystem ID for all
emulated devices.  Which will make the qemu devices look pretty much
like in the laptop case: all PCI subsystem IDs will point to qemu by
default.

If a driver emulates a very specific piece of hardware where it has to
emulate more than just the PCI chip, it can overwrite the PCI subsystem
ID without problems.  The es1370 driver does that for example.

Right now the patch uses the vendor ID 0xfffa.  XenSource grabbed a
temporary ID (before they got one official assigned) from the 0xfffx
space too.  We'll better get something official though, to avoid
clashes.  We could try to get a vendor ID assigned (no idea how
difficuilt and/or expensive that would be).  Or we could try get get a
device ID range from a vendor (like the qumranet-sponsored IDs for
virtio ...).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c |   11 +++++++++++
 hw/pci.h |    3 +++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index bc55989..f2d0c4b 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;
+static uint16_t pci_default_sub_vendor_id = PCI_VENDOR_ID_QEMU;
+static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU_DEFAULT;
 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 f518e5e..d0b8a3e 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -34,6 +34,9 @@ struct pci_config_header {
 
 extern target_phys_addr_t pci_mem_base;
 
+#define PCI_VENDOR_ID_QEMU               0xfffa  /* FIXME: get one assigned */
+#define PCI_SUBDEVICE_ID_QEMU_DEFAULT    0x0001
+
 typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
                                 uint32_t address, uint32_t data, int len);
 typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
-- 
1.5.5.1

[Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Gerd Hoffmann]

This makes use of the new pci_config_header struct in pci.c,
squashing a bunch of casts and hard-coded magic numbers.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/pci.c |   64 +++++++++++++++++++++++++++++++-------------------------------
 1 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index f2d0c4b..b142709 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -199,8 +199,9 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
                             uint32_t size, int type,
                             PCIMapIORegionFunc *map_func)
 {
+    struct pci_config_header *conf = (void*)pci_dev->config;
     PCIIORegion *r;
-    uint32_t addr;
+    le32 *addr;
 
     if ((unsigned int)region_num >= PCI_NUM_REGIONS)
         return;
@@ -210,11 +211,11 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
     r->type = type;
     r->map_func = map_func;
     if (region_num == PCI_ROM_SLOT) {
-        addr = 0x30;
+        addr = &conf->rom_addr;
     } else {
-        addr = 0x10 + region_num * 4;
+        addr = conf->base_address_regs + region_num;
     }
-    *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
+    *addr = cpu_to_le32(type);
 }
 
 static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
@@ -224,23 +225,24 @@ static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
 
 static void pci_update_mappings(PCIDevice *d)
 {
+    struct pci_config_header *conf = (void*)d->config;
     PCIIORegion *r;
     int cmd, i;
-    uint32_t last_addr, new_addr, config_ofs;
+    uint32_t last_addr, new_addr;
+    le32 *config_addr;
 
-    cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
+    cmd = le16_to_cpu(conf->command);
     for(i = 0; i < PCI_NUM_REGIONS; i++) {
         r = &d->io_regions[i];
         if (i == PCI_ROM_SLOT) {
-            config_ofs = 0x30;
+            config_addr = &conf->rom_addr;
         } else {
-            config_ofs = 0x10 + i * 4;
+            config_addr = conf->base_address_regs + i;
         }
         if (r->size != 0) {
             if (r->type & PCI_ADDRESS_SPACE_IO) {
                 if (cmd & PCI_COMMAND_IO) {
-                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
-                                                         config_ofs));
+                    new_addr = le32_to_cpu(*config_addr);
                     new_addr = new_addr & ~(r->size - 1);
                     last_addr = new_addr + r->size - 1;
                     /* NOTE: we have only 64K ioports on PC */
@@ -253,8 +255,7 @@ static void pci_update_mappings(PCIDevice *d)
                 }
             } else {
                 if (cmd & PCI_COMMAND_MEMORY) {
-                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
-                                                         config_ofs));
+                    new_addr = le32_to_cpu(*config_addr);
                     /* the ROM slot has a specific enable bit */
                     if (i == PCI_ROM_SLOT && !(new_addr & 1))
                         goto no_mem_map;
@@ -568,13 +569,14 @@ static pci_class_desc pci_class_descriptions[] =
 
 static void pci_info_device(PCIDevice *d)
 {
+    struct pci_config_header *conf = (void*)d->config;
     int i, class;
     PCIIORegion *r;
     pci_class_desc *desc;
 
     term_printf("  Bus %2d, device %3d, function %d:\n",
            d->bus->bus_num, d->devfn >> 3, d->devfn & 7);
-    class = le16_to_cpu(*((uint16_t *)(d->config + PCI_CLASS_DEVICE)));
+    class = conf->class << 8 | conf->subclass;
     term_printf("    ");
     desc = pci_class_descriptions;
     while (desc->desc && class != desc->class)
@@ -585,11 +587,11 @@ static void pci_info_device(PCIDevice *d)
         term_printf("Class %04x", class);
     }
     term_printf(": PCI device %04x:%04x\n",
-           le16_to_cpu(*((uint16_t *)(d->config + PCI_VENDOR_ID))),
-           le16_to_cpu(*((uint16_t *)(d->config + PCI_DEVICE_ID))));
+                le16_to_cpu(conf->vendor_id),
+                le16_to_cpu(conf->device_id));
 
-    if (d->config[PCI_INTERRUPT_PIN] != 0) {
-        term_printf("      IRQ %d.\n", d->config[PCI_INTERRUPT_LINE]);
+    if (conf->interrupt_line != 0) {
+        term_printf("      IRQ %d.\n", conf->interrupt_line);
     }
     if (class == 0x0604) {
         term_printf("      BUS %d.\n", d->config[0x19]);
@@ -686,25 +688,23 @@ static void pci_bridge_write_config(PCIDevice *d,
 PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint32_t id,
                         pci_map_irq_fn map_irq, const char *name)
 {
+    struct pci_config_header *conf;
     PCIBridge *s;
     s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge),
                                          devfn, NULL, pci_bridge_write_config);
-    s->dev.config[0x00] = id >> 16;
-    s->dev.config[0x01] = id >> 24;
-    s->dev.config[0x02] = id; // device_id
-    s->dev.config[0x03] = id >> 8;
-    s->dev.config[0x04] = 0x06; // command = bus master, pci mem
-    s->dev.config[0x05] = 0x00;
-    s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error
-    s->dev.config[0x07] = 0x00; // status = fast devsel
-    s->dev.config[0x08] = 0x00; // revision
-    s->dev.config[0x09] = 0x00; // programming i/f
-    s->dev.config[0x0A] = 0x04; // class_sub = PCI to PCI bridge
-    s->dev.config[0x0B] = 0x06; // class_base = PCI_bridge
-    s->dev.config[0x0D] = 0x10; // latency_timer
-    s->dev.config[0x0E] = 0x81; // header_type
-    s->dev.config[0x1E] = 0xa0; // secondary status
+    conf = (void*)s->dev.config;
+    conf->vendor_id = cpu_to_le16(id >> 16);
+    conf->device_id = cpu_to_le16(id & 0xffff);
+    conf->command   = cpu_to_le16(0x0006); // bus master, pci mem
+    conf->status    = cpu_to_le16(0x00a0); // fast back-to-back, 66MHz, no error, fast devsel
+    conf->revision      = 0x00;
+    conf->api           = 0x00;
+    conf->subclass      = 0x04; // PCI to PCI bridge
+    conf->class         = 0x06; // PCI_bridge
+    conf->latency_timer = 0x10;
+    conf->header_type   = 0x81;
 
+    s->dev.config[0x1E] = 0xa0; // secondary status
     s->bus = pci_register_secondary_bus(&s->dev, map_irq);
     return s->bus;
 }
-- 
1.5.5.1

Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Anthony Liguori]

Gerd Hoffmann wrote:
> This makes use of the new pci_config_header struct in pci.c,
> squashing a bunch of casts and hard-coded magic numbers.
>   

I thought the last bit of feedback that you received from Paul was that 
it seemed like a waste to go through the trouble of introducing this PCI 
config structure but then forcing the users to do explicit endian 
casting.  I agree with this and was expecting the updated patches to do 
the endian conversion automatically.

Any reasons for not doing it this way?

Regards,

Anthony Liguori

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/pci.c |   64 +++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index f2d0c4b..b142709 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -199,8 +199,9 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
>                              uint32_t size, int type,
>                              PCIMapIORegionFunc *map_func)
>  {
> +    struct pci_config_header *conf = (void*)pci_dev->config;
>      PCIIORegion *r;
> -    uint32_t addr;
> +    le32 *addr;
>  
>      if ((unsigned int)region_num >= PCI_NUM_REGIONS)
>          return;
> @@ -210,11 +211,11 @@ void pci_register_io_region(PCIDevice *pci_dev, int region_num,
>      r->type = type;
>      r->map_func = map_func;
>      if (region_num == PCI_ROM_SLOT) {
> -        addr = 0x30;
> +        addr = &conf->rom_addr;
>      } else {
> -        addr = 0x10 + region_num * 4;
> +        addr = conf->base_address_regs + region_num;
>      }
> -    *(uint32_t *)(pci_dev->config + addr) = cpu_to_le32(type);
> +    *addr = cpu_to_le32(type);
>  }
>  
>  static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
> @@ -224,23 +225,24 @@ static target_phys_addr_t pci_to_cpu_addr(target_phys_addr_t addr)
>  
>  static void pci_update_mappings(PCIDevice *d)
>  {
> +    struct pci_config_header *conf = (void*)d->config;
>      PCIIORegion *r;
>      int cmd, i;
> -    uint32_t last_addr, new_addr, config_ofs;
> +    uint32_t last_addr, new_addr;
> +    le32 *config_addr;
>  
> -    cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
> +    cmd = le16_to_cpu(conf->command);
>      for(i = 0; i < PCI_NUM_REGIONS; i++) {
>          r = &d->io_regions[i];
>          if (i == PCI_ROM_SLOT) {
> -            config_ofs = 0x30;
> +            config_addr = &conf->rom_addr;
>          } else {
> -            config_ofs = 0x10 + i * 4;
> +            config_addr = conf->base_address_regs + i;
>          }
>          if (r->size != 0) {
>              if (r->type & PCI_ADDRESS_SPACE_IO) {
>                  if (cmd & PCI_COMMAND_IO) {
> -                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
> -                                                         config_ofs));
> +                    new_addr = le32_to_cpu(*config_addr);
>                      new_addr = new_addr & ~(r->size - 1);
>                      last_addr = new_addr + r->size - 1;
>                      /* NOTE: we have only 64K ioports on PC */
> @@ -253,8 +255,7 @@ static void pci_update_mappings(PCIDevice *d)
>                  }
>              } else {
>                  if (cmd & PCI_COMMAND_MEMORY) {
> -                    new_addr = le32_to_cpu(*(uint32_t *)(d->config +
> -                                                         config_ofs));
> +                    new_addr = le32_to_cpu(*config_addr);
>                      /* the ROM slot has a specific enable bit */
>                      if (i == PCI_ROM_SLOT && !(new_addr & 1))
>                          goto no_mem_map;
> @@ -568,13 +569,14 @@ static pci_class_desc pci_class_descriptions[] =
>  
>  static void pci_info_device(PCIDevice *d)
>  {
> +    struct pci_config_header *conf = (void*)d->config;
>      int i, class;
>      PCIIORegion *r;
>      pci_class_desc *desc;
>  
>      term_printf("  Bus %2d, device %3d, function %d:\n",
>             d->bus->bus_num, d->devfn >> 3, d->devfn & 7);
> -    class = le16_to_cpu(*((uint16_t *)(d->config + PCI_CLASS_DEVICE)));
> +    class = conf->class << 8 | conf->subclass;
>      term_printf("    ");
>      desc = pci_class_descriptions;
>      while (desc->desc && class != desc->class)
> @@ -585,11 +587,11 @@ static void pci_info_device(PCIDevice *d)
>          term_printf("Class %04x", class);
>      }
>      term_printf(": PCI device %04x:%04x\n",
> -           le16_to_cpu(*((uint16_t *)(d->config + PCI_VENDOR_ID))),
> -           le16_to_cpu(*((uint16_t *)(d->config + PCI_DEVICE_ID))));
> +                le16_to_cpu(conf->vendor_id),
> +                le16_to_cpu(conf->device_id));
>  
> -    if (d->config[PCI_INTERRUPT_PIN] != 0) {
> -        term_printf("      IRQ %d.\n", d->config[PCI_INTERRUPT_LINE]);
> +    if (conf->interrupt_line != 0) {
> +        term_printf("      IRQ %d.\n", conf->interrupt_line);
>      }
>      if (class == 0x0604) {
>          term_printf("      BUS %d.\n", d->config[0x19]);
> @@ -686,25 +688,23 @@ static void pci_bridge_write_config(PCIDevice *d,
>  PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint32_t id,
>                          pci_map_irq_fn map_irq, const char *name)
>  {
> +    struct pci_config_header *conf;
>      PCIBridge *s;
>      s = (PCIBridge *)pci_register_device(bus, name, sizeof(PCIBridge),
>                                           devfn, NULL, pci_bridge_write_config);
> -    s->dev.config[0x00] = id >> 16;
> -    s->dev.config[0x01] = id >> 24;
> -    s->dev.config[0x02] = id; // device_id
> -    s->dev.config[0x03] = id >> 8;
> -    s->dev.config[0x04] = 0x06; // command = bus master, pci mem
> -    s->dev.config[0x05] = 0x00;
> -    s->dev.config[0x06] = 0xa0; // status = fast back-to-back, 66MHz, no error
> -    s->dev.config[0x07] = 0x00; // status = fast devsel
> -    s->dev.config[0x08] = 0x00; // revision
> -    s->dev.config[0x09] = 0x00; // programming i/f
> -    s->dev.config[0x0A] = 0x04; // class_sub = PCI to PCI bridge
> -    s->dev.config[0x0B] = 0x06; // class_base = PCI_bridge
> -    s->dev.config[0x0D] = 0x10; // latency_timer
> -    s->dev.config[0x0E] = 0x81; // header_type
> -    s->dev.config[0x1E] = 0xa0; // secondary status
> +    conf = (void*)s->dev.config;
> +    conf->vendor_id = cpu_to_le16(id >> 16);
> +    conf->device_id = cpu_to_le16(id & 0xffff);
> +    conf->command   = cpu_to_le16(0x0006); // bus master, pci mem
> +    conf->status    = cpu_to_le16(0x00a0); // fast back-to-back, 66MHz, no error, fast devsel
> +    conf->revision      = 0x00;
> +    conf->api           = 0x00;
> +    conf->subclass      = 0x04; // PCI to PCI bridge
> +    conf->class         = 0x06; // PCI_bridge
> +    conf->latency_timer = 0x10;
> +    conf->header_type   = 0x81;
>  
> +    s->dev.config[0x1E] = 0xa0; // secondary status
>      s->bus = pci_register_secondary_bus(&s->dev, map_irq);
>      return s->bus;
>  }
>   

Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Gerd Hoffmann]

Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> This makes use of the new pci_config_header struct in pci.c,
>> squashing a bunch of casts and hard-coded magic numbers.
>>   
> 
> I thought the last bit of feedback that you received from Paul was that
> it seemed like a waste to go through the trouble of introducing this PCI
> config structure but then forcing the users to do explicit endian
> casting.  I agree with this and was expecting the updated patches to do
> the endian conversion automatically.
> 
> Any reasons for not doing it this way?

I'm not sure what you are expecting here.  The idea to introduce
gcc-checkable byteordered types (via struct trick) was rejected.  So I
have le32 & friends just typedef'ed to the corrosponding int*_t types
and use the existing cpu_*() byteorder macros.  The new types are
basically documentation now.

Endian swapping must still be done by the users.  Lots of casts can be
dropped though, which IMHO improves the readability alot (random chunk
picked from patch):

-    cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
+    cmd = le16_to_cpu(conf->command);

I'm not aware of any way to do the byteswapping here automatically.

Except maybe introducing accessor functions/macros for each and every
struct element and hide the byteswapping macro call therein.  Which in
turn makes providing the struct to pci drivers sort of pointless ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Anthony Liguori]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
>   
> I'm not sure what you are expecting here.  The idea to introduce
> gcc-checkable byteordered types (via struct trick) was rejected.  So I
> have le32 & friends just typedef'ed to the corrosponding int*_t types
> and use the existing cpu_*() byteorder macros.  The new types are
> basically documentation now.
>
> Endian swapping must still be done by the users.  Lots of casts can be
> dropped though, which IMHO improves the readability alot (random chunk
> picked from patch):
>
> -    cmd = le16_to_cpu(*(uint16_t *)(d->config + PCI_COMMAND));
> +    cmd = le16_to_cpu(conf->command);
>
> I'm not aware of any way to do the byteswapping here automatically.
>   

Because the config structure is loaded after pci_register device and 
because the MMIO callbacks into the config space?  If you look at 
pci_default_read_config(), it actually does conversion from le to host 
CPU because that's what MMIO callbacks expect.  If you simply removed 
this, it would work.  Of course, you'll have to audit each caller of 
pci_register_device() and make sure their config's get changed too.

Regards,

Anthony LIguori

> Except maybe introducing accessor functions/macros for each and every
> struct element and hide the byteswapping macro call therein.  Which in
> turn makes providing the struct to pci drivers sort of pointless ...
>
> cheers,
>   Gerd
>   

Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Gerd Hoffmann]

Anthony Liguori wrote:
> Because the config structure is loaded after pci_register device and
> because the MMIO callbacks into the config space?  If you look at
> pci_default_read_config(), it actually does conversion from le to host
> CPU because that's what MMIO callbacks expect.

Yep.

> If you simply removed
> this, it would work.

Ah, *now* I see what you mean.

Well.  That assumes the guests always use the correct in{b,w,l}
instruction to access the config space values, i.e. never ever try to
use two inb reads for a 16bit value for example.  Or a inw for the lower
16 bits of a 32bit value because they know the high bits are zero anyway.

I have my doubts that this is a sane expectation.

> Of course, you'll have to audit each caller of
> pci_register_device() and make sure their config's get changed too.

Sure, that would be a *big* task & patch.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Anthony Liguori]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
>   
>> Because the config structure is loaded after pci_register device and
>> because the MMIO callbacks into the config space?  If you look at
>> pci_default_read_config(), it actually does conversion from le to host
>> CPU because that's what MMIO callbacks expect.
>>     
>
> Yep.
>
>   
>> If you simply removed
>> this, it would work.
>>     
>
> Ah, *now* I see what you mean.
>
> Well.  That assumes the guests always use the correct in{b,w,l}
> instruction to access the config space values, i.e. never ever try to
> use two inb reads for a 16bit value for example.  Or a inw for the lower
> 16 bits of a 32bit value because they know the high bits are zero anyway.
>
> I have my doubts that this is a sane expectation.
>   

The config space seems organized by data size so you could always just 
take care of this in the accessors.  That is, if you get a byte access 
to a 32-bit value, you would do something like:

(cpu_to_le32(d->config + (addr & ~3)) >> (8 * (addr % 4))) & 0xFF;

Not the prettiest thing in the world, but it would work.

>> Of course, you'll have to audit each caller of
>> pci_register_device() and make sure their config's get changed too.
>>     
>
> Sure, that would be a *big* task & patch.
>   

Actually, it's not.  I did a quick grep, noone seems to pass in a 
config_read accessor.  Only config_write is ever passed and it only 
seems to be to used to add callbacks when particular config values are 
written.  pci_default_config_write is still used to do the actual write.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>   

Re: [Qemu-devel] [PATCH 4/4] pci: use pci_config_header in pci.c

[Gerd Hoffmann]

Anthony Liguori wrote:
> Gerd Hoffmann wrote:
>> Well.  That assumes the guests always use the correct in{b,w,l}
>> instruction to access the config space values, i.e. never ever try to
>> use two inb reads for a 16bit value for example.  Or a inw for the lower
>> 16 bits of a 32bit value because they know the high bits are zero anyway.
>>
>> I have my doubts that this is a sane expectation.
> 
> The config space seems organized by data size so you could always just
> take care of this in the accessors.  That is, if you get a byte access
> to a 32-bit value, you would do something like:
> 
> (cpu_to_le32(d->config + (addr & ~3)) >> (8 * (addr % 4))) & 0xFF;
> 
> Not the prettiest thing in the world, but it would work.

-ETOO_MUCH_MAGIC.

I want make the code more readable, that would go into the wrong
direction IMHO.  I think it is much more sane to explicitly call the
byteswapping macros in the code.  And add sparse checking support to
qemu so we have some way to check correctness without taking the
struct-trick route.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Christoph Hellwig]

On Wed, Oct 01, 2008 at 04:12:52PM +0200, Gerd Hoffmann wrote:
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  bswap.h      |    7 +++++++
>  hw/usb-net.c |    2 --
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/bswap.h b/bswap.h
> index 523d805..b294d96 100644
> --- a/bswap.h
> +++ b/bswap.h
> @@ -206,4 +206,11 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
>  #undef le_bswaps
>  #undef be_bswaps
>  
> +typedef uint16_t le16;
> +typedef uint32_t le32;
> +typedef uint64_t le64;
> +typedef uint16_t be16;
> +typedef uint32_t be32;
> +typedef uint64_t be64;

Any chance you could add sparse __bitwise__ declarations so one could
use spare to check for using these types correctly?

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

Christoph Hellwig wrote:

>> +typedef uint16_t le16;
>> +typedef uint32_t le32;
>> +typedef uint64_t le64;
>> +typedef uint16_t be16;
>> +typedef uint32_t be32;
>> +typedef uint64_t be64;
> 
> Any chance you could add sparse __bitwise__ declarations so one could
> use spare to check for using these types correctly?

Is there any good documentation on that?

Looking at the linux kernel sources I find the bitwise stuff depend on
the __CHECKER__ and __CHECK_ENDIAN__ defines.  I can't find references
to these in the sparse man page and sparse FAQ (sparse 0.4.1 as shipped
by fedora 9).

I can somehow guess what these two defines do.  __CHECKER__ probably is
set by sparse, and __CHECK_ENDIAN__ by the -Wbitwise switch.  But I
can't see any obvious reason why include/linux/types.h defines both
__bitwise__ and __bitwise.  I'd simply use something along the lines ...

  #if defined(__CHECKER__) && defined(__CHECK_ENDIAN__)
  # define __bitwise__ __attribute__((bitwise))
  #else
  # define __bitwise__
  #endif

  typedef uint16_t __bitwise__ le16;
  [ ... ]

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Christoph Hellwig]

On Thu, Oct 02, 2008 at 02:46:41PM +0200, Gerd Hoffmann wrote:
> Christoph Hellwig wrote:
> 
> >> +typedef uint16_t le16;
> >> +typedef uint32_t le32;
> >> +typedef uint64_t le64;
> >> +typedef uint16_t be16;
> >> +typedef uint32_t be32;
> >> +typedef uint64_t be64;
> > 
> > Any chance you could add sparse __bitwise__ declarations so one could
> > use spare to check for using these types correctly?
> 
> Is there any good documentation on that?

I don't think so.  But I've recently added support for it in the XFS
userspace tools, so I have some experience.

> 
> Looking at the linux kernel sources I find the bitwise stuff depend on
> the __CHECKER__ and __CHECK_ENDIAN__ defines.  I can't find references
> to these in the sparse man page and sparse FAQ (sparse 0.4.1 as shipped
> by fedora 9).
> 
> I can somehow guess what these two defines do.  __CHECKER__ probably is
> set by sparse, and __CHECK_ENDIAN__ by the -Wbitwise switch.  But I
> can't see any obvious reason why include/linux/types.h defines both
> __bitwise__ and __bitwise.  I'd simply use something along the lines ...

Actually __CHECKER__ is set by sparse.  __CHECK_ENDIAN__ is set on the
make command line when checking for endianess bugs.  For xfsprogs I
defined the bitwise annotations unconditionally because we made sure
to not have any of this warnings left (this was quite easy becaus a lot
of the code came from the kernel and was already properly annotated)

These are the snipplets from xfsprogs that would be useful for qemu:

#ifdef __CHECKER__
#define __bitwise	__attribute__((bitwise))
#define __force		__attribute__((force))
#else
#define __bitwise
#define __force
#endif

The __force is needed for the actual swab macros to turn the __bitwise
annotated type back into the normal so it doesn't warn.  For XFS these
conversion macros looks the same as the kernel ones:

#ifdef XFS_NATIVE_HOST
#define cpu_to_be16(val)       ((__force __be16)(__u16)(val))
#define cpu_to_be32(val)       ((__force __be32)(__u32)(val))
#define cpu_to_be64(val)       ((__force __be64)(__u64)(val))
#define be16_to_cpu(val)       ((__force __u16)(__be16)(val))
#define be32_to_cpu(val)       ((__force __u32)(__be32)(val))
#define be64_to_cpu(val)       ((__force __u64)(__be64)(val))
#else
#define cpu_to_be16(val)       ((__force __be16)__swab16((__u16)(val)))
#define cpu_to_be32(val)       ((__force __be32)__swab32((__u32)(val)))
#define cpu_to_be64(val)       ((__force __be64)__swab64((__u64)(val)))
#define be16_to_cpu(val)       (__swab16((__force __u16)(__be16)(val)))
#define be32_to_cpu(val)       (__swab32((__force __u32)(__be32)(val)))
#define be64_to_cpu(val)       (__swab64((__force __u64)(__be64)(val)))
#endif

and then you define the types as __bitwise:

typedef __u16 __bitwise                __be16;
typedef __u32 __bitwise                __be32;
typedef __u64 __bitwise                __be64;

for qemu you'll need slight changes in the names, and also little
endian variants which we don't have in XFS.

Note that the biggest hurdle for xfsprogs was to convince libtool
that cgcc, the gcc wrapper for invoking sparse actually is a C compiler,
but that should be mood for qemu.

Note that sparse will probably complain about a lot of things in
qemu, so you'll have to add a lot -Wno-foo options in addition to
-Wbitwise.

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

Christoph Hellwig wrote:
> Actually __CHECKER__ is set by sparse.  __CHECK_ENDIAN__ is set on the
> make command line when checking for endianess bugs.  For xfsprogs I
> defined the bitwise annotations unconditionally because we made sure
> to not have any of this warnings left (this was quite easy becaus a lot
> of the code came from the kernel and was already properly annotated)

Thanks.  I still can't see the point of __CHECK_ENDIAN__ though, as
-W(no-)bitwise should have the very same effect.  Historical reasons?

> These are the snipplets from xfsprogs that would be useful for qemu:
> 
> #ifdef __CHECKER__
> #define __bitwise	__attribute__((bitwise))
> #define __force		__attribute__((force))
> #else
> #define __bitwise
> #define __force
> #endif

> The __force is needed for the actual swab macros to turn the __bitwise
> annotated type back into the normal so it doesn't warn.  For XFS these
> conversion macros looks the same as the kernel ones:
> 
> #ifdef XFS_NATIVE_HOST
> #define cpu_to_be16(val)       ((__force __be16)(__u16)(val))
[ ... ]
> #else
> #define cpu_to_be16(val)       ((__force __be16)__swab16((__u16)(val)))
[ ... ]
> #endif
> 
> and then you define the types as __bitwise:
> 
> typedef __u16 __bitwise                __be16;
[ ... ]

I see.

> for qemu you'll need slight changes in the names, and also little
> endian variants which we don't have in XFS.

Sure.

> Note that the biggest hurdle for xfsprogs was to convince libtool
> that cgcc, the gcc wrapper for invoking sparse actually is a C compiler,
> but that should be mood for qemu.

Nevertheless it must be winded up in configure and the build system.
Shouldn't be hard though.  How fast is sparse?  Would it be sane to run
it by default in case we find cgcc installed on the system?  Or should
it better be opt-in?

> Note that sparse will probably complain about a lot of things in
> qemu, so you'll have to add a lot -Wno-foo options in addition to
> -Wbitwise.

Even -Wbitwise will probably generate tons of warnings initially.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Christoph Hellwig]

On Thu, Oct 02, 2008 at 03:15:19PM +0200, Gerd Hoffmann wrote:
> > Actually __CHECKER__ is set by sparse.  __CHECK_ENDIAN__ is set on the
> > make command line when checking for endianess bugs.  For xfsprogs I
> > defined the bitwise annotations unconditionally because we made sure
> > to not have any of this warnings left (this was quite easy becaus a lot
> > of the code came from the kernel and was already properly annotated)
> 
> Thanks.  I still can't see the point of __CHECK_ENDIAN__ though, as
> -W(no-)bitwise should have the very same effect.  Historical reasons?

Me neither.  I guess it simply predates -Wbitwise.

> > Note that the biggest hurdle for xfsprogs was to convince libtool
> > that cgcc, the gcc wrapper for invoking sparse actually is a C compiler,
> > but that should be mood for qemu.
> 
> Nevertheless it must be winded up in configure and the build system.
> Shouldn't be hard though.  How fast is sparse?  Would it be sane to run
> it by default in case we find cgcc installed on the system?  Or should
> it better be opt-in?

cgcc is a drop-in wrapper that calls both gcc and sparse.  So you just
set your compiler to cgcc before configure or in the Makefile and then
both sparse and gcc get run.

sparse is extremly fast and barely noticeable compared to the speed
gcc takes to compile a file, but of course you have two fork exec
wait circles instead of one so some overhead is noticeable when
compiling lots of small files.

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

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

Christoph Hellwig wrote:
> cgcc is a drop-in wrapper that calls both gcc and sparse.  So you just
> set your compiler to cgcc before configure or in the Makefile and then
> both sparse and gcc get run.

The attached configure patch enabled that in case sparse is installed.
Should also get cross-builds right.  Only buildsystem stuff for now, no
bitwise annotations (yet).

Oh joy, I get sparse warnings and errors(!) for the system headers then.
Disabled the most noisy stuff for now.

Comments?
  Gerd


[-- Attachment #2: qemu-sparse.diff --]
[-- Type: text/plain, Size: 1887 bytes --]

diff --git a/configure b/configure
index 9c8ed81..3f3e0f7 100755
--- a/configure
+++ b/configure
@@ -85,6 +85,7 @@ case "$cpu" in
   ;;
 esac
 gprof="no"
+sparse="yes"
 bigendian="no"
 mingw32="no"
 EXESUF=""
@@ -286,6 +287,8 @@ for opt do
   ;;
   --audio-drv-list=*) audio_drv_list="$optarg"
   ;;
+  --disable-sparse) sparse="no"
+  ;;
   --disable-vnc-tls) vnc_tls="no"
   ;;
   --disable-slirp) slirp="no"
@@ -425,6 +428,7 @@ echo "  --host-cc=CC             use C compiler CC [$host_cc] for dyngen etc."
 echo "  --make=MAKE              use specified make [$make]"
 echo "  --install=INSTALL        use specified install [$install]"
 echo "  --static                 enable static build [$static]"
+echo "  --disable-sparse         disable sparse checker"
 echo "  --disable-werror         disable compilation abort on warning"
 echo "  --disable-sdl            disable SDL"
 echo "  --enable-cocoa           enable COCOA (Mac OS X only)"
@@ -536,6 +540,10 @@ EOF
     fi
 fi
 
+if test ! -x "$(which cgcc 2>/dev/null)"; then
+    sparse="no"
+fi
+
 #
 # Solaris specific configure tool chain decisions
 #
@@ -1007,6 +1015,7 @@ echo "host CPU          $cpu"
 echo "host big endian   $bigendian"
 echo "target list       $target_list"
 echo "gprof enabled     $gprof"
+echo "sparse enabled    $sparse"
 echo "profiler          $profiler"
 echo "static build      $static"
 echo "-Werror enabled   $werror"
@@ -1156,6 +1165,11 @@ case "$cpu" in
     exit 1
   ;;
 esac
+if test "$sparse" = "yes" ; then
+  echo "CC      := REAL_CC=\"\$(CC)\" cgcc"       >> $config_mak
+  echo "HOST_CC := REAL_CC=\"\$(HOST_CC)\" cgcc"  >> $config_mak
+  echo "CFLAGS  += -Wbitwise -Wno-transparent-union -Wno-old-initializer -Wno-non-pointer-null" >> $config_mak
+fi
 if test "$bigendian" = "yes" ; then
   echo "WORDS_BIGENDIAN=yes" >> $config_mak
   echo "#define WORDS_BIGENDIAN 1" >> $config_h

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Christoph Hellwig wrote:
>   
>> cgcc is a drop-in wrapper that calls both gcc and sparse.  So you just
>> set your compiler to cgcc before configure or in the Makefile and then
>> both sparse and gcc get run.
>>     
>
> The attached configure patch enabled that in case sparse is installed.
> Should also get cross-builds right.  Only buildsystem stuff for now, no
> bitwise annotations (yet).
>
> Oh joy, I get sparse warnings and errors(!) for the system headers then.
> Disabled the most noisy stuff for now.
>
> Comments?
>   

Good idea, but perhaps it should be disabled by default instead of 
enabled by default?

Regards,

Anthony Liguori

>   Gerd
>
>   

Re: [Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

Anthony Liguori wrote:
> Gerd Hoffmann wrote:
> Good idea, but perhaps it should be disabled by default instead of
> enabled by default?

I'd try default-enabled first.  Given that sparse isn't installed by
default by distros I think it is ok:  When people installed it
intentionally they probably welcome seeing it used automatically ;)

cheers,
  Gerd

[Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 bswap.h      |    7 +++++++
 hw/usb-net.c |    2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/bswap.h b/bswap.h
index 523d805..b294d96 100644
--- a/bswap.h
+++ b/bswap.h
@@ -206,4 +206,11 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 #undef le_bswaps
 #undef be_bswaps
 
+typedef uint16_t le16;
+typedef uint32_t le32;
+typedef uint64_t le64;
+typedef uint16_t be16;
+typedef uint32_t be32;
+typedef uint64_t be64;
+
 #endif /* BSWAP_H */
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 63edfd5..23ae805 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -326,8 +326,6 @@ enum {
 #define OID_PNP_REMOVE_WAKE_UP_PATTERN	0xfd010104
 #define OID_PNP_ENABLE_WAKE_UP		0xfd010106
 
-typedef uint32_t le32;
-
 typedef struct rndis_init_msg_type {
     le32 MessageType;
     le32 MessageLength;
-- 
1.5.5.1

[Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

This sets a default PCI subsystem ID for all emulated PCI devices.  PCI
specs require this, so do it.  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.

The defaults are pre-filled by pci_register_device().  Individual
drivers can overwrite it of course when setting up the config space
for the emulated device.

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 |    5 +++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index bc55989..1304dae 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 = PCI_VENDOR_ID_QEMU;
+uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU_DEFAULT;
 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 f518e5e..3c6976a 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -33,6 +33,11 @@ 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;
+
+#define PCI_VENDOR_ID_QEMU               0xfffa  /* FIXME: get one assigned */
+#define PCI_SUBDEVICE_ID_QEMU_DEFAULT    0x0001
 
 typedef void PCIConfigWriteFunc(PCIDevice *pci_dev,
                                 uint32_t address, uint32_t data, int len);
-- 
1.5.5.1

Re: [Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Paul Brook]

On Wednesday 10 September 2008, Gerd Hoffmann wrote:
> +extern uint16_t pci_default_sub_vendor_id;
> +extern uint16_t pci_default_sub_device_id;

Why are these being exported?

Paul

Re: [Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

Paul Brook wrote:
> On Wednesday 10 September 2008, Gerd Hoffmann wrote:
>> +extern uint16_t pci_default_sub_vendor_id;
>> +extern uint16_t pci_default_sub_device_id;
> 
> Why are these being exported?

So you can change the defaults.  Main user for that I have in mind is
Xen.  They have added a subsystem id (5853:0001, XenSource vendor ID) to
some of the devices (by patching the indiviudual drivers, not by adding
a global default like this patch does).

I expect they probably want to keep the XenSource subsystem ID instead
of going with the qemu default.  So the idea is that we can have "if
(using_xen) { make 5853:0001 the default }" in the initialization path
some day.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/4] add byteordered types to qemu.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 bswap.h      |    7 +++++++
 hw/usb-net.c |    2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/bswap.h b/bswap.h
index 523d805..b294d96 100644
--- a/bswap.h
+++ b/bswap.h
@@ -206,4 +206,11 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 #undef le_bswaps
 #undef be_bswaps
 
+typedef uint16_t le16;
+typedef uint32_t le32;
+typedef uint64_t le64;
+typedef uint16_t be16;
+typedef uint32_t be32;
+typedef uint64_t be64;
+
 #endif /* BSWAP_H */
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 27dea10..447c0ce 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -326,8 +326,6 @@ enum {
 #define OID_PNP_REMOVE_WAKE_UP_PATTERN	0xfd010104
 #define OID_PNP_ENABLE_WAKE_UP		0xfd010106
 
-typedef uint32_t le32;
-
 typedef struct rndis_init_msg_type {
     le32 MessageType;
     le32 MessageLength;
-- 
1.5.5.1

[Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

This sets a default PCI subsystem ID for all emulated PCI devices.  PCI
specs require this, so do it.  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.

The defaults are pre-filled by pci_register_device().  Individual
drivers can overwrite it of course when setting up the config space
for the emulated device.

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 f518e5e..28e8fb5 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 3/4] pci: add default pci subsystem id for all devices.

[Anthony Liguori]

Gerd Hoffmann wrote:
> This sets a default PCI subsystem ID for all emulated PCI devices.  PCI
> specs require this, so do it.  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.
>
> The defaults are pre-filled by pci_register_device().  Individual
> drivers can overwrite it of course when setting up the config space
> for the emulated device.
>
> TODO: get an official vendor ID assigned, or borrow one (maybe
>       Qumranet which already sponsors the virtio IDs ???).
>   

Does the subvendor ID have to be an official vendor ID?  I thought that 
the subvendor ID could be defined by the vendor as whatever it wants..

> 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;
>   

Probably should just be a define.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

Anthony Liguori wrote:
>> TODO: get an official vendor ID assigned, or borrow one (maybe
>>       Qumranet which already sponsors the virtio IDs ???).
> 
> Does the subvendor ID have to be an official vendor ID?

I think so, as far I know there is no difference between vendor and
subvendor ID in that respect.

> I thought that
> the subvendor ID could be defined by the vendor as whatever it wants..

Each vendor can assign the device (and subdevice) IDs freely.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Anthony Liguori wrote:
>   
>>> TODO: get an official vendor ID assigned, or borrow one (maybe
>>>       Qumranet which already sponsors the virtio IDs ???).
>>>       
>> Does the subvendor ID have to be an official vendor ID?
>>     
>
> I think so, as far I know there is no difference between vendor and
> subvendor ID in that respect.
>
>   
>> I thought that
>> the subvendor ID could be defined by the vendor as whatever it wants..
>>     
>
> Each vendor can assign the device (and subdevice) IDs freely.
>   

It's worth looking into because in virtio, we currently define the 
subvendor ID to be not the PCI vendor ID space.  I'll try to dig up a 
copy of the PCI spec and confirm.  If it's not required to be the PCI 
vendor ID space, then this patch is unnecessary.  We can just set it to 
a non-0 value of our choosing.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
>
>   

Re: [Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

  Hi,

>> Each vendor can assign the device (and subdevice) IDs freely.
> 
> It's worth looking into because in virtio, we currently define the
> subvendor ID to be not the PCI vendor ID space.  I'll try to dig up a
> copy of the PCI spec and confirm.  If it's not required to be the PCI
> vendor ID space, then this patch is unnecessary.  We can just set it to
> a non-0 value of our choosing.

Ehrm, no.  I'll try a bit more verbose ...

Each PCI device has a PCI ID.  It should also have a PCI Subsystem ID.
The PCI subsystem ID wasn't mandatory in early PCI spec revisions, IIRC
2.0 added that requirement.  Thats why you usually get away without a
PCI subsystem ID, although it isn't correct.

Both IDs have a 16bit vendor and a 16bit device part.  The vendor IDs
are handed out by the PCI SIG.  The device IDs are assigned by the
vendor owning the vendor ID.

The PCI ID tells you which PCI chip is used, whereas the the PCI
Subsystem ID identifies the actual device (created using the PCI chip).

Example #1:  TV cards.  All TV cards using the bt878 chipset (first and
only one on the market for a few years in the 90-ies) have the same PCI
ID, the one of the bt878 chip.  But each TV card has different PCI
Subsystem IDs, which can be used to figure what the actual TV card is.

Example #2:  Laptops.  It is quite common to find the PCI Subsytem ID
pointing to the laptop vendor and model.  So the PCI ID says
'this is a intel ich7 ide controller', whereas the the PCI Subsystem ID
says 'this ich7 sits in a lenovo thinkpad', like this:

--------- cut here ----------
[root@zweiblum ~]# lspci -vs1f.1
00:1f.1 IDE interface: Intel Corporation 82801G (ICH7 Family) IDE
Controller (rev 02) (prog-if 8a [Master SecP PriP])
        Subsystem: Lenovo ThinkPad T60/R60 series
        [ ... more stuff snipped ... ]
--------- cut here ----------

In many cases it is enougth to know the PCI ID to handle a device
correctly.  Sometimes a device driver must identify the exact piece of
hardware (via PCI Subsystem ID) though.


Ok, now to qemu.  Right now the emulated PCI devices have no PCI
subsystem ID, only the PCI ID.  The discussed patch sets a default PCI
subsystem ID for all emulated devices.  Which will make the qemu devices
look pretty much like in the laptop case: all PCI subsystem IDs will
point to qemu by default.

If a driver emulates a very specific piece of hardware where it has to
emulate more than just the PCI chip, it can overwrite the PCI subsystem
ID without problems.  The es1370 driver does that for example.


Right now the patch uses the vendor ID 0xfffa.  XenSource grabbed a
temporary ID (before they got one official assigned) from the 0xfffx
space too.  We'll better get something official though, to avoid
clashes.  We could try to get a vendor ID assigned (no idea how
difficuilt and/or expensive that would be).  Or we could try get get a
device ID range from a vendor (like the qumranet-sponsored IDs for
virtio ...).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

Gerd Hoffmann wrote:
>   Hi,
> 
>>> Each vendor can assign the device (and subdevice) IDs freely.
>> It's worth looking into because in virtio, we currently define the
>> subvendor ID to be not the PCI vendor ID space.  I'll try to dig up a
>> copy of the PCI spec and confirm.  If it's not required to be the PCI
>> vendor ID space, then this patch is unnecessary.  We can just set it to
>> a non-0 value of our choosing.

Ping.  What is the status of this?  Did you find time to check the PCI
specs?  Did the explanation I've tried below the trick?  Any other
outstanding issues with the patches?  I'm not aware of any ...

cheers,
  Gerd

> Ehrm, no.  I'll try a bit more verbose ...
> 
> Each PCI device has a PCI ID.  It should also have a PCI Subsystem ID.
> The PCI subsystem ID wasn't mandatory in early PCI spec revisions, IIRC
> 2.0 added that requirement.  Thats why you usually get away without a
> PCI subsystem ID, although it isn't correct.
> 
> Both IDs have a 16bit vendor and a 16bit device part.  The vendor IDs
> are handed out by the PCI SIG.  The device IDs are assigned by the
> vendor owning the vendor ID.
> 
> The PCI ID tells you which PCI chip is used, whereas the the PCI
> Subsystem ID identifies the actual device (created using the PCI chip).
> 
> Example #1:  TV cards.  All TV cards using the bt878 chipset (first and
> only one on the market for a few years in the 90-ies) have the same PCI
> ID, the one of the bt878 chip.  But each TV card has different PCI
> Subsystem IDs, which can be used to figure what the actual TV card is.
> 
> Example #2:  Laptops.  It is quite common to find the PCI Subsytem ID
> pointing to the laptop vendor and model.  So the PCI ID says
> 'this is a intel ich7 ide controller', whereas the the PCI Subsystem ID
> says 'this ich7 sits in a lenovo thinkpad', like this:
> 
> --------- cut here ----------
> [root@zweiblum ~]# lspci -vs1f.1
> 00:1f.1 IDE interface: Intel Corporation 82801G (ICH7 Family) IDE
> Controller (rev 02) (prog-if 8a [Master SecP PriP])
>         Subsystem: Lenovo ThinkPad T60/R60 series
>         [ ... more stuff snipped ... ]
> --------- cut here ----------
> 
> In many cases it is enougth to know the PCI ID to handle a device
> correctly.  Sometimes a device driver must identify the exact piece of
> hardware (via PCI Subsystem ID) though.
> 
> 
> Ok, now to qemu.  Right now the emulated PCI devices have no PCI
> subsystem ID, only the PCI ID.  The discussed patch sets a default PCI
> subsystem ID for all emulated devices.  Which will make the qemu devices
> look pretty much like in the laptop case: all PCI subsystem IDs will
> point to qemu by default.
> 
> If a driver emulates a very specific piece of hardware where it has to
> emulate more than just the PCI chip, it can overwrite the PCI subsystem
> ID without problems.  The es1370 driver does that for example.
> 
> 
> Right now the patch uses the vendor ID 0xfffa.  XenSource grabbed a
> temporary ID (before they got one official assigned) from the 0xfffx
> space too.  We'll better get something official though, to avoid
> clashes.  We could try to get a vendor ID assigned (no idea how
> difficuilt and/or expensive that would be).  Or we could try get get a
> device ID range from a vendor (like the qumranet-sponsored IDs for
> virtio ...).
> 
> cheers,
>   Gerd
> 

[Qemu-devel] [PATCH 1/4] add byteordered types and accessor functions to qemu.

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 bswap.h      |   29 +++++++++++
 hw/usb-net.c |  158 +++++++++++++++++++++++++++++----------------------------
 2 files changed, 109 insertions(+), 78 deletions(-)

diff --git a/bswap.h b/bswap.h
index 523d805..feb2d6b 100644
--- a/bswap.h
+++ b/bswap.h
@@ -206,4 +206,33 @@ static inline void cpu_to_be32wu(uint32_t *p, uint32_t v)
 #undef le_bswaps
 #undef be_bswaps
 
+/* byteordered types and accessors */
+
+typedef struct { uint16_t le; } le16 __attribute__((__aligned__(2)));
+typedef struct { uint32_t le; } le32;
+typedef struct { uint64_t le; } le64;
+typedef struct { uint16_t be; } be16 __attribute__((__aligned__(2)));
+typedef struct { uint32_t be; } be32;
+typedef struct { uint64_t be; } be64;
+
+static inline uint16_t read_le16(le16 le) { return le16_to_cpu(le.le); }
+static inline uint32_t read_le32(le32 le) { return le32_to_cpu(le.le); }
+static inline uint64_t read_le64(le64 le) { return le64_to_cpu(le.le); }
+static inline uint16_t read_be16(be16 be) { return be16_to_cpu(be.be); }
+static inline uint32_t read_be32(be32 be) { return be32_to_cpu(be.be); }
+static inline uint64_t read_be64(be64 be) { return be64_to_cpu(be.be); }
+
+static inline le16 write_le16(uint16_t cpu) \
+	{ le16 ret = { .le = cpu_to_le16(cpu) }; return ret; }
+static inline le32 write_le32(uint32_t cpu) \
+	{ le32 ret = { .le = cpu_to_le32(cpu) }; return ret; }
+static inline le64 write_le64(uint64_t cpu) \
+	{ le64 ret = { .le = cpu_to_le64(cpu) }; return ret; }
+static inline be16 write_be16(uint16_t cpu) \
+	{ be16 ret = { .be = cpu_to_be16(cpu) }; return ret; }
+static inline be32 write_be32(uint32_t cpu) \
+	{ be32 ret = { .be = cpu_to_be32(cpu) }; return ret; }
+static inline be64 write_be64(uint64_t cpu) \
+	{ be64 ret = { .be = cpu_to_be64(cpu) }; return ret; }
+
 #endif /* BSWAP_H */
diff --git a/hw/usb-net.c b/hw/usb-net.c
index 27dea10..dbeb54a 100644
--- a/hw/usb-net.c
+++ b/hw/usb-net.c
@@ -326,8 +326,6 @@ enum {
 #define OID_PNP_REMOVE_WAKE_UP_PATTERN	0xfd010104
 #define OID_PNP_ENABLE_WAKE_UP		0xfd010106
 
-typedef uint32_t le32;
-
 typedef struct rndis_init_msg_type {
     le32 MessageType;
     le32 MessageLength;
@@ -629,6 +627,7 @@ static int ndis_query(USBNetState *s, uint32_t oid,
                       size_t outlen)
 {
     unsigned int i, count;
+    le32 *outbuf32 = (void*)outbuf;
 
     switch (oid) {
     /* general oids (table 4-1) */
@@ -636,47 +635,47 @@ static int ndis_query(USBNetState *s, uint32_t oid,
     case OID_GEN_SUPPORTED_LIST:
         count = sizeof(oid_supported_list) / sizeof(uint32_t);
         for (i = 0; i < count; i++)
-            ((le32 *) outbuf)[i] = cpu_to_le32(oid_supported_list[i]);
+            outbuf32[i] = write_le32(oid_supported_list[i]);
         return sizeof(oid_supported_list);
 
     /* mandatory */
     case OID_GEN_HARDWARE_STATUS:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MEDIA_SUPPORTED:
-        *((le32 *) outbuf) = cpu_to_le32(s->medium);
+        outbuf32[0] = write_le32(s->medium);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MEDIA_IN_USE:
-        *((le32 *) outbuf) = cpu_to_le32(s->medium);
+        outbuf32[0] = write_le32(s->medium);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MAXIMUM_FRAME_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(ETH_FRAME_LEN);
+        outbuf32[0] = write_le32(ETH_FRAME_LEN);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_LINK_SPEED:
-        *((le32 *) outbuf) = cpu_to_le32(s->speed);
+        outbuf32[0] = write_le32(s->speed);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_TRANSMIT_BLOCK_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(ETH_FRAME_LEN);
+        outbuf32[0] = write_le32(ETH_FRAME_LEN);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RECEIVE_BLOCK_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(ETH_FRAME_LEN);
+        outbuf32[0] = write_le32(ETH_FRAME_LEN);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_VENDOR_ID:
-        *((le32 *) outbuf) = cpu_to_le32(s->vendorid);
+        outbuf32[0] = write_le32(s->vendorid);
         return sizeof(le32);
 
     /* mandatory */
@@ -685,30 +684,30 @@ static int ndis_query(USBNetState *s, uint32_t oid,
         return strlen(outbuf) + 1;
 
     case OID_GEN_VENDOR_DRIVER_VERSION:
-        *((le32 *) outbuf) = cpu_to_le32(1);
+        outbuf32[0] = write_le32(1);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_CURRENT_PACKET_FILTER:
-        *((le32 *) outbuf) = cpu_to_le32(s->filter);
+        outbuf32[0] = write_le32(s->filter);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MAXIMUM_TOTAL_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(RNDIS_MAX_TOTAL_SIZE);
+        outbuf32[0] = write_le32(RNDIS_MAX_TOTAL_SIZE);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_MEDIA_CONNECT_STATUS:
-        *((le32 *) outbuf) = cpu_to_le32(s->media_state);
+        outbuf32[0] = write_le32(s->media_state);
         return sizeof(le32);
 
     case OID_GEN_PHYSICAL_MEDIUM:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     case OID_GEN_MAC_OPTIONS:
-        *((le32 *) outbuf) = cpu_to_le32(
+        outbuf32[0] = write_le32(
                         NDIS_MAC_OPTION_RECEIVE_SERIALIZED |
                         NDIS_MAC_OPTION_FULL_DUPLEX);
         return sizeof(le32);
@@ -716,27 +715,27 @@ static int ndis_query(USBNetState *s, uint32_t oid,
     /* statistics OIDs (table 4-2) */
     /* mandatory */
     case OID_GEN_XMIT_OK:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RCV_OK:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_XMIT_ERROR:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RCV_ERROR:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_GEN_RCV_NO_BUFFER:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* ieee802.3 OIDs (table 4-3) */
@@ -752,12 +751,12 @@ static int ndis_query(USBNetState *s, uint32_t oid,
 
     /* mandatory */
     case OID_802_3_MULTICAST_LIST:
-        *((le32 *) outbuf) = cpu_to_le32(0xe0000000);
+        outbuf32[0] = write_le32(0xe0000000);
         return sizeof(le32);
 
     /* mandatory */
     case OID_802_3_MAXIMUM_LIST_SIZE:
-        *((le32 *) outbuf) = cpu_to_le32(1);
+        outbuf32[0] = write_le32(1);
         return sizeof(le32);
 
     case OID_802_3_MAC_OPTIONS:
@@ -766,17 +765,17 @@ static int ndis_query(USBNetState *s, uint32_t oid,
     /* ieee802.3 statistics OIDs (table 4-4) */
     /* mandatory */
     case OID_802_3_RCV_ERROR_ALIGNMENT:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_802_3_XMIT_ONE_COLLISION:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     /* mandatory */
     case OID_802_3_XMIT_MORE_COLLISIONS:
-        *((le32 *) outbuf) = cpu_to_le32(0);
+        outbuf32[0] = write_le32(0);
         return sizeof(le32);
 
     default:
@@ -789,9 +788,11 @@ static int ndis_query(USBNetState *s, uint32_t oid,
 static int ndis_set(USBNetState *s, uint32_t oid,
                 uint8_t *inbuf, unsigned int inlen)
 {
+    le32 *inbuf32 = (void*)inbuf;
+
     switch (oid) {
     case OID_GEN_CURRENT_PACKET_FILTER:
-        s->filter = le32_to_cpup((le32 *) inbuf);
+        s->filter = read_le32(inbuf32[0]);
         if (s->filter) {
             s->rndis_state = RNDIS_DATA_INITIALIZED;
         } else {
@@ -850,20 +851,20 @@ static int rndis_init_response(USBNetState *s, rndis_init_msg_type *buf)
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_INITIALIZE_CMPLT);
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_init_cmplt_type));
+    resp->MessageType = write_le32(RNDIS_INITIALIZE_CMPLT);
+    resp->MessageLength = write_le32(sizeof(rndis_init_cmplt_type));
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
-    resp->MajorVersion = cpu_to_le32(RNDIS_MAJOR_VERSION);
-    resp->MinorVersion = cpu_to_le32(RNDIS_MINOR_VERSION);
-    resp->DeviceFlags = cpu_to_le32(RNDIS_DF_CONNECTIONLESS);
-    resp->Medium = cpu_to_le32(RNDIS_MEDIUM_802_3);
-    resp->MaxPacketsPerTransfer = cpu_to_le32(1);
-    resp->MaxTransferSize = cpu_to_le32(ETH_FRAME_LEN +
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
+    resp->MajorVersion = write_le32(RNDIS_MAJOR_VERSION);
+    resp->MinorVersion = write_le32(RNDIS_MINOR_VERSION);
+    resp->DeviceFlags = write_le32(RNDIS_DF_CONNECTIONLESS);
+    resp->Medium = write_le32(RNDIS_MEDIUM_802_3);
+    resp->MaxPacketsPerTransfer = write_le32(1);
+    resp->MaxTransferSize = write_le32(ETH_FRAME_LEN +
                     sizeof(struct rndis_packet_msg_type) + 22);
-    resp->PacketAlignmentFactor = cpu_to_le32(0);
-    resp->AFListOffset = cpu_to_le32(0);
-    resp->AFListSize = cpu_to_le32(0);
+    resp->PacketAlignmentFactor = write_le32(0);
+    resp->AFListOffset = write_le32(0);
+    resp->AFListSize = write_le32(0);
     return 0;
 }
 
@@ -877,12 +878,12 @@ static int rndis_query_response(USBNetState *s,
     int infobuflen;
     unsigned int resplen;
 
-    bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
-    buflen = le32_to_cpu(buf->InformationBufferLength);
+    bufoffs = read_le32(buf->InformationBufferOffset) + 8;
+    buflen = read_le32(buf->InformationBufferLength);
     if (bufoffs + buflen > length)
         return USB_RET_STALL;
 
-    infobuflen = ndis_query(s, le32_to_cpu(buf->OID),
+    infobuflen = ndis_query(s, read_le32(buf->OID),
                             bufoffs + (uint8_t *) buf, buflen, infobuf,
                             sizeof(infobuf));
     resplen = sizeof(rndis_query_cmplt_type) +
@@ -891,22 +892,22 @@ static int rndis_query_response(USBNetState *s,
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_QUERY_CMPLT);
+    resp->MessageType = write_le32(RNDIS_QUERY_CMPLT);
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->MessageLength = cpu_to_le32(resplen);
+    resp->MessageLength = write_le32(resplen);
 
     if (infobuflen < 0) {
         /* OID not supported */
-        resp->Status = cpu_to_le32(RNDIS_STATUS_NOT_SUPPORTED);
-        resp->InformationBufferLength = cpu_to_le32(0);
-        resp->InformationBufferOffset = cpu_to_le32(0);
+        resp->Status = write_le32(RNDIS_STATUS_NOT_SUPPORTED);
+        resp->InformationBufferLength = write_le32(0);
+        resp->InformationBufferOffset = write_le32(0);
         return 0;
     }
 
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
     resp->InformationBufferOffset =
-            cpu_to_le32(infobuflen ? sizeof(rndis_query_cmplt_type) - 8 : 0);
-    resp->InformationBufferLength = cpu_to_le32(infobuflen);
+            write_le32(infobuflen ? sizeof(rndis_query_cmplt_type) - 8 : 0);
+    resp->InformationBufferLength = write_le32(infobuflen);
     memcpy(resp + 1, infobuf, infobuflen);
 
     return 0;
@@ -923,22 +924,22 @@ static int rndis_set_response(USBNetState *s,
     if (!resp)
         return USB_RET_STALL;
 
-    bufoffs = le32_to_cpu(buf->InformationBufferOffset) + 8;
-    buflen = le32_to_cpu(buf->InformationBufferLength);
+    bufoffs = read_le32(buf->InformationBufferOffset) + 8;
+    buflen = read_le32(buf->InformationBufferLength);
     if (bufoffs + buflen > length)
         return USB_RET_STALL;
 
-    ret = ndis_set(s, le32_to_cpu(buf->OID),
+    ret = ndis_set(s, read_le32(buf->OID),
                     bufoffs + (uint8_t *) buf, buflen);
-    resp->MessageType = cpu_to_le32(RNDIS_SET_CMPLT);
+    resp->MessageType = write_le32(RNDIS_SET_CMPLT);
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_set_cmplt_type));
+    resp->MessageLength = write_le32(sizeof(rndis_set_cmplt_type));
     if (ret < 0) {
         /* OID not supported */
-        resp->Status = cpu_to_le32(RNDIS_STATUS_NOT_SUPPORTED);
+        resp->Status = write_le32(RNDIS_STATUS_NOT_SUPPORTED);
         return 0;
     }
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
 
     return 0;
 }
@@ -951,10 +952,10 @@ static int rndis_reset_response(USBNetState *s, rndis_reset_msg_type *buf)
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_RESET_CMPLT);
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_reset_cmplt_type));
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
-    resp->AddressingReset = cpu_to_le32(1); /* reset information */
+    resp->MessageType = write_le32(RNDIS_RESET_CMPLT);
+    resp->MessageLength = write_le32(sizeof(rndis_reset_cmplt_type));
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
+    resp->AddressingReset = write_le32(1); /* reset information */
 
     return 0;
 }
@@ -968,10 +969,10 @@ static int rndis_keepalive_response(USBNetState *s,
     if (!resp)
         return USB_RET_STALL;
 
-    resp->MessageType = cpu_to_le32(RNDIS_KEEPALIVE_CMPLT);
-    resp->MessageLength = cpu_to_le32(sizeof(rndis_keepalive_cmplt_type));
+    resp->MessageType = write_le32(RNDIS_KEEPALIVE_CMPLT);
+    resp->MessageLength = write_le32(sizeof(rndis_keepalive_cmplt_type));
     resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
-    resp->Status = cpu_to_le32(RNDIS_STATUS_SUCCESS);
+    resp->Status = write_le32(RNDIS_STATUS_SUCCESS);
 
     return 0;
 }
@@ -979,10 +980,10 @@ static int rndis_keepalive_response(USBNetState *s,
 static int rndis_parse(USBNetState *s, uint8_t *data, int length)
 {
     uint32_t msg_type, msg_length;
-    le32 *tmp = (le32 *) data;
+    le32 *data32 = (le32 *) data;
 
-    msg_type = le32_to_cpup(tmp++);
-    msg_length = le32_to_cpup(tmp++);
+    msg_type = read_le32(data32[0]);
+    msg_length = read_le32(data32[1]);
 
     switch (msg_type) {
     case RNDIS_INITIALIZE_MSG:
@@ -1210,12 +1211,13 @@ static int usb_net_handle_control(USBDevice *dev, int request, int value,
 static int usb_net_handle_statusin(USBNetState *s, USBPacket *p)
 {
     int ret = 8;
+    le32 *data32 = (void*)p->data;
 
     if (p->len < 8)
         return USB_RET_STALL;
 
-    ((le32 *) p->data)[0] = cpu_to_le32(1);
-    ((le32 *) p->data)[1] = cpu_to_le32(0);
+    data32[0] = write_le32(1);
+    data32[1] = write_le32(0);
     if (!s->rndis_resp.tqh_first)
         ret = USB_RET_NAK;
 
@@ -1311,12 +1313,12 @@ static int usb_net_handle_dataout(USBNetState *s, USBPacket *p)
         }
         return ret;
     }
-    len = le32_to_cpu(msg->MessageLength);
+    len = read_le32(msg->MessageLength);
     if (s->out_ptr < 8 || s->out_ptr < len)
         return ret;
-    if (le32_to_cpu(msg->MessageType) == RNDIS_PACKET_MSG) {
-        uint32_t offs = 8 + le32_to_cpu(msg->DataOffset);
-        uint32_t size = le32_to_cpu(msg->DataLength);
+    if (read_le32(msg->MessageType) == RNDIS_PACKET_MSG) {
+        uint32_t offs = 8 + read_le32(msg->DataOffset);
+        uint32_t size = read_le32(msg->DataLength);
         if (offs + size <= len)
             qemu_send_packet(s->vc, s->out_buf + offs, size);
     }
@@ -1383,10 +1385,10 @@ static void usbnet_receive(void *opaque, const uint8_t *buf, int size)
             return;
 
         memset(msg, 0, sizeof(struct rndis_packet_msg_type));
-        msg->MessageType = cpu_to_le32(RNDIS_PACKET_MSG);
-        msg->MessageLength = cpu_to_le32(size + sizeof(struct rndis_packet_msg_type));
-        msg->DataOffset = cpu_to_le32(sizeof(struct rndis_packet_msg_type) - 8);
-        msg->DataLength = cpu_to_le32(size);
+        msg->MessageType = write_le32(RNDIS_PACKET_MSG);
+        msg->MessageLength = write_le32(size + sizeof(struct rndis_packet_msg_type));
+        msg->DataOffset = write_le32(sizeof(struct rndis_packet_msg_type) - 8);
+        msg->DataLength = write_le32(size);
         /* msg->OOBDataOffset;
          * msg->OOBDataLength;
          * msg->NumOOBDataElements;
-- 
1.5.5.1

[Qemu-devel] [PATCH 3/4] pci: add default pci subsystem id for all devices.

[Gerd Hoffmann]

This sets a default PCI subsystem ID for all emulated PCI devices.  PCI
specs require this, so do it.  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.

The defaults are pre-filled by pci_register_device().  Individual
drivers can overwrite it of course when setting up the config space
for the emulated device.

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..15e8173 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 = write_le16(pci_default_sub_vendor_id);
+    conf->sub_device_id = write_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 f518e5e..28e8fb5 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