[Qemu-devel] [patch 1/2] add pci config space struct

[Qemu-devel] [patch 1/2] add pci config space struct

[Gerd Hoffmann]

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

  Hi,

This patch adds a struct for the pci config header make it easier to
deal with it.  Comes from qemu-xen.

cheers,
  Gerd

[-- Attachment #2: 0018-pci-add-config-space-struct-from-qemu-xen.patch --]
[-- Type: text/plain, Size: 1341 bytes --]

>From b46342e5ea1c1d47b040becc2352ea03c9b3a33d Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 26 Aug 2008 12:11:53 +0200
Subject: [PATCH] pci: add config space struct (from qemu-xen).


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..2b1ebba 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -6,6 +6,32 @@
 
 /* PCI bus */
 
+struct pci_config_header {
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint16_t command;
+    uint16_t 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 */
+    uint32_t base_address_regs[6];
+    uint32_t reserved1;
+    uint16_t sub_vendor_id;
+    uint16_t sub_device_id;
+    uint32_t rom_addr;
+    uint32_t reserved3;
+    uint32_t 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

Re: [Qemu-devel] [patch 1/2] add pci config space struct

[Avi Kivity]

Gerd Hoffmann wrote:
>   Hi,
>
> This patch adds a struct for the pci config header make it easier to
> deal with it.  Comes from qemu-xen.
>
> cheers,
>   Gerd
>   
> +struct pci_config_header {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint16_t command;
> +    uint16_t 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 */
> +    uint32_t base_address_regs[6];
> +    uint32_t reserved1;
> +    uint16_t sub_vendor_id;
> +    uint16_t sub_device_id;
> +    uint32_t rom_addr;
> +    uint32_t reserved3;
> +    uint32_t reserved4;
> +    uint8_t  interrupt_line;
> +    uint8_t  interrupt_pin;
> +    uint8_t  min_gnt;
> +    uint8_t  max_lat;
> +};

Shouldn't little-endian types be used here to force the users to use 
little-endian accessors?

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

Re: [Qemu-devel] [patch 1/2] add pci config space struct

[Gerd Hoffmann]

Avi Kivity wrote:
> Gerd Hoffmann wrote:
>>   +struct pci_config_header {
>> +    uint16_t vendor_id;

> Shouldn't little-endian types be used here to force the users to use
> little-endian accessors?

Didn't notice qemu has that.  Which header file I should check?  There
is nothing in bswap.h ...

thanks,
  Gerd

Re: [Qemu-devel] [patch 1/2] add pci config space struct

[Avi Kivity]

Gerd Hoffmann wrote:
>> Shouldn't little-endian types be used here to force the users to use
>> little-endian accessors?
>>     
>
> Didn't notice qemu has that.  Which header file I should check?  There
> is nothing in bswap.h ...
>   

I don't think qemu has them; it should though.

I'd define le32 (and friends) as a struct to ensure it is impossible to 
misuse.

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

Re: [Qemu-devel] [patch 1/2] add pci config space struct

[Gerd Hoffmann]

Avi Kivity wrote:
> Gerd Hoffmann wrote:
>>> Shouldn't little-endian types be used here to force the users to use
>>> little-endian accessors?
>>>     
>>
>> Didn't notice qemu has that.  Which header file I should check?  There
>> is nothing in bswap.h ...
> 
> I don't think qemu has them; it should though.

Adding byteordered types is a completely separate issue IMHO.

> I'd define le32 (and friends) as a struct to ensure it is impossible to
> misuse.

Sure, that is the usual way ;)

I expected you suggest to just use something which is present already
though.

cheers,
  Gerd

Re: [Qemu-devel] [patch 1/2] add pci config space struct

[Paul Brook]

On Tuesday 26 August 2008, Gerd Hoffmann wrote:
>   Hi,
>
> This patch adds a struct for the pci config header make it easier to
> deal with it.  Comes from qemu-xen.

Adding a nice easy-to-use structure, then requiring all devices do 
byteswapping themselves (negating most of the easy-to-use benefit) seems less 
than ideal.

Paul

Re: [Qemu-devel] [patch 1/2] add pci config space struct

[Gerd Hoffmann]

Paul Brook wrote:
> On Tuesday 26 August 2008, Gerd Hoffmann wrote:
>>   Hi,
>>
>> This patch adds a struct for the pci config header make it easier to
>> deal with it.  Comes from qemu-xen.
> 
> Adding a nice easy-to-use structure, then requiring all devices do 
> byteswapping themselves (negating most of the easy-to-use benefit) seems less 
> than ideal.

Byteswapping must be done anyway, with or without the struct.  Using the
struct allows to avoid casting all over the place though.  I'll go
switch over the config header struct to use the byteswapped types, then
repost together with some pci.c functions changed to use the new struct.
 IMHO it makes the code more readable due to hardcoded magic numbers and
casts going away.

cheers,
  Gerd