Indirection Cleanup
cover Indirection Cleanup
This patch series clean up "half" converted qemu drivers that had changed from:
struct FOOState
to
typedef PCIFOOState {
PCIDevice dev;
FOOState foo;
} PCIFOOState;It just moves PCIDevice to be the 1st field of FOOState.
Once there, other cleanups were done:
a - pci_dev pointer from FOOState to PCIFOOState is removed, jsut use s->dev
The field is leave only in the drivers that also emulate isa.
b- Once there, transformo
PCIFOOState *s = (PCIFOOState *)pci_dev
to
PCIFOOState *s = DO_UPCAST(PCIFOOState, dev, pci_dev)
where pci_dev is a PCI_DEVICE.
c- again, once there, remove all the casts from void * (they are not
needed since '89)
PCIFOOState *s = (PCIFOOState *)opaque;
to
PCIFOOState *s = opaque;
d- Start of vga.c cleanup. It is not trivial, as just now
VGAState == VGACommonState, functions need to be changed to use the right value.
ToDo:
- pcnet: It needs a different approach, because it can be both a PCIDevice or a SysBus device.
- vga: It have to separate the common part for the not common part, problems now is that VGAState is used for both the common state and the standard vga. To make things more interesting, different bits are "inherited" from vga in different devices:
- cirrus_vga
- vmware_vga
- blizzard (vga that is not pci)
- vga common state has a pci_dev pointer, that is only needed by std vga, as cirrus_vga stores it (with this patches) otherplace, blizzard is not pci .... you get the idea
- I guess some other driver is missing, but my fast grep didn't found it.
Later, Juan.
Juan Quintela (22):
eepro100: convert casts to DO_UPCAST()
eepro100: cast a void * makes no sense
eepro100: Remove unused indirection of PCIDevice
es1370: Remove unused indirection of PCIES1370State and ES1370State
ne2000: remove casts from void *
ne2000: pci_dev has this very value with the right type
ne2000: Remove unneeded double indirection of PCINE2000State
ne2000: change pci_dev to is_pci
pci: remove casts from void *
rtl8139: Remove unneeded double indirection of PCIRTL8139State
rtl8139: remove pointless cast from void *
lsi53c895a: remove pointless cast from void *
lsi53c895a: use DO_UPCAST to cast from PCIDevice
lsi53c895a: rename PCIDevice field from pci_dev to dev (consistence)
lsi53c895a: LSIState is a PCIDevice is a DeviceHost
usb-ohci: Remove unneeded double indirection of OHCIPCIState
cirrus_vga: Remove unneeded double indirection of PCICirrusVGAState
cirrus_vga: remove pointless cast from void *
cirrus_vga: change use of pci_dev for is_pci
Introduce vga_common_reset() to be able to typcheck vga_reset()
vga: Rename vga_state -> vga
Everything outside of vga.c should use VGACommonState
hw/cirrus_vga.c | 59 ++++++++++++++++++++++--------------------------- hw/eepro100.c | 65 ++++++++++++++++++++++-------------------------------- hw/es1370.c | 31 ++++++++----------------- hw/lsi53c895a.c | 65 ++++++++++++++++++++++++++++--------------------------- hw/ne2000.c | 41 ++++++++++++++-------------------- hw/pci.c | 8 +++--- hw/rtl8139.c | 42 ++++++++++++---------------------- hw/usb-ohci.c | 34 +++++++++++----------------- hw/vga.c | 26 +++++++++++++-------- hw/vga_int.h | 12 ++++------ hw/vmware_vga.c | 4 +- 11 files changed, 170 insertions(+), 217 deletions(-)
PATCH 1/22 eepro100: convert casts to DO_UPCAST()
--- hw/eepro100.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
hw/eepro100.c +5 -5
@@ -1,4 +1,4 @@ -/* +/* * QEMU i8255x (PRO100) emulation * * Copyright (c) 2006-2007 Stefan Weil
@@ -1346,7 +1346,7 @@ typedef struct PCIEEPRO100State {static void pci_map (PCIDevice * pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev);EEPRO100State *s = &d->eepro100; logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",
@@ -1420,7 +1420,7 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {static void pci_mmio_map (PCIDevice * pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev);logout ("region %d, addr=0x%08x, size=0x%08x, type=%d\n" , region_num, addr, size, type);
@@ -1720,7 +1720,7 @@ static void nic_cleanup(VLANClientState *vc)static int pci_nic_uninit (PCIDevice *dev) { -PCIEEPRO100State *d = (PCIEEPRO100State *) dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, dev);EEPRO100State *s = &d->eepro100;cpu_unregister_io_memory (s->mmio_index);
@@ -1730,7 +1730,7 @@ static int pci_nic_uninit(PCIDevice *dev)static void nic_init (PCIDevice *pci_dev,uint32_t device) { -PCIEEPRO100State *d = (PCIEEPRO100State *)pci_dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev);EEPRO100State *s;logout ("\n" );
Signature · 1 line
1.6.2.5
Re: [Qemu-devel] [PATCH 01/22] eepro100: convert casts to DO_UPCAST()
Quoted 11 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>--- hw/eepro100.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)hw/eepro100.c +0
-0
hw/eepro100.c line 4
Quoted 5 lines
hw/eepro100.c +1
-1@@ -1,4 +1,4 @@ -/* +/* * QEMU i8255x (PRO100) emulation * * Copyright (c) 2006-2007 Stefan Weil
Please don't change formatting of multiline comments like this.
Quoted 37 lines
hw/eepro100.c +4
-4@@ -1346,7 +1346,7 @@ typedef struct PCIEEPRO100State {static void pci_map (PCIDevice * pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev);EEPRO100State *s = &d->eepro100; logout("region %d, addr=0x%08x, size=0x%08x, type=%d\n",@@ -1420,7 +1420,7 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {static void pci_mmio_map (PCIDevice * pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIEEPRO100State *d = (PCIEEPRO100State *) pci_dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev);logout ("region %d, addr=0x%08x, size=0x%08x, type=%d\n" , region_num, addr, size, type);@@ -1720,7 +1720,7 @@ static void nic_cleanup(VLANClientState *vc)static int pci_nic_uninit (PCIDevice *dev) { -PCIEEPRO100State *d = (PCIEEPRO100State *) dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, dev);EEPRO100State *s = &d->eepro100;cpu_unregister_io_memory (s->mmio_index);@@ -1730,7 +1730,7 @@ static int pci_nic_uninit(PCIDevice *dev)static void nic_init (PCIDevice *pci_dev,uint32_t device) { -PCIEEPRO100State *d = (PCIEEPRO100State *)pci_dev; +PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev);EEPRO100State *s;logout ("\n" );
[Qemu-devel] Re: [PATCH 01/22] eepro100: convert casts to DO_UPCAST()
Quoted 6 lines · Stefan Weil
Quoted 5 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>--- hw/eepro100.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
Quoted 13 lines
hw/eepro100.c line 4
Quoted 11 lines
hw/eepro100.c +1
-1@@ -1,4 +1,4 @@ -/* +/* * QEMU i8255x (PRO100) emulation * * Copyright (c) 2006-2007 Stefan Weil Please don't change formatting of multiline comments like this.
Opp, that was a typo. Sorry.
later, Juan.
PATCH 2/22 eepro100: cast a void * makes no sense
In reply to Juan Quintela--- hw/eepro100.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
hw/eepro100.c +3 -3
@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)static void nic_reset (void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {
@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t sizestatic int nic_load (QEMUFile * f,void *opaque,int version_id) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;int ret;
@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)static void nic_save (QEMUFile * f,void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;if (s->pci_dev)
Signature · 1 line
1.6.2.5
Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense
Quoted 29 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>--- hw/eepro100.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)hw/eepro100.c +2
-2@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)static void nic_reset (void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t sizestatic int nic_load (QEMUFile * f,void *opaque,int version_id) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;int ret;
hw/eepro100.c static int nic_load(QEMUFile * f, void *opaque, int version_id) · line 1640
Quoted 8 lines
hw/eepro100.c +1
-1@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)static void nic_save (QEMUFile * f,void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;if (s->pci_dev)
I wrote these type casts, and I think they make sense. In C++ code, they are even mandatory.
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
So please don't apply this patch.
Regards
Stefan Weil
[Qemu-devel] Re: [PATCH 02/22] eepro100: cast a void * makes no sense
Quoted 9 lines · Stefan Weil
hw/eepro100.c static void nic_selective_reset(EEPRO100State * s) · line 513
Quoted 8 lines · Juan Quintela
static void
nic_reset(void *opaque)
{hw/eepro100.c +1
-1-EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {
Hi
Quoted 1 line
I wrote these type casts, and I think they make sense.
In C, they made no sense. they are a nop.
In qemu, they are not used almost anywhere.
Quoted 1 line
In C++ code, they are even mandatory.
This is no C++.
Quoted 2 lines
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
And they are? I don't see what they buy us.
Quoted 2 lines
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
If it is possible to have a consistent code base, with little effort, I prefer to do so.
Quoted 1 line
So please don't apply this patch.
I think it should be applied (qemu sense), but you do eepro100 work, and I don't do/plan to do more eepro100 work.
I guess it is your call.
Later, Juan.
Re: [Qemu-devel] Re: [PATCH 02/22] eepro100: cast a void * makes no sense
Quoted 3 lines · Juan Quintela
I think it should be applied (qemu sense), but you do eepro100 work, and I don't do/plan to do more eepro100 work.
I agree with Juan.
Overall consistency in the code base is extremely important. Stefan, if you disagree with this style, you should submit a cleanup patch that converts every occurrence of this idiom.
Regards,
Anthony Liguori
Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense
In reply to Stefan WeilQuoted 36 lines · Stefan Weil
Quoted 35 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>--- hw/eepro100.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)hw/eepro100.c +3
-3@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)static void nic_reset (void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t sizestatic int nic_load (QEMUFile * f,void *opaque,int version_id) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;int ret;@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)static void nic_save (QEMUFile * f,void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;
Quoted 5 lines
hw/eepro100.c static int nic_load(QEMUFile * f, void *opaque, int version_id) · line 1640
Quoted 2 lines
hw/eepro100.c +0
-0if (s->pci_dev)I wrote these type casts, and I think they make sense. In C++ code, they are even mandatory.
Yes, but this isn't C++.
Quoted 5 lines
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
I respectfully disagree. Casts from "void *" to "T *" are pure noise. Getting into the habit of writing noise casts runs the risk of silencing warnings that flag real type errors.
[Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 50 lines · Markus Armbruster
Quoted 38 lines · Stefan Weil
Quoted 37 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>---
hw/eepro100.c| 6 +++---1 files changed, 3 insertions(+), 3 deletions(-)hw/eepro100.c +3
-3@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)static void nic_reset (void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t sizestatic int nic_load (QEMUFile * f,void *opaque,int version_id) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;int ret;@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)static void nic_save (QEMUFile * f,void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;Quoted 4 lines
Quoted 2 lines
if (
s->pci_dev)I wrote these type casts, and I think they make sense. In C++ code, they are even mandatory.
Yes, but this isn't C++.
Quoted 3 lines
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
Quoted 6 lines
Quoted 2 lines
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
I respectfully disagree. Casts from "void *" to "T *" are pure noise. Getting into the habit of writing noise casts runs the risk of silencing warnings that flag real type errors.
Hello
Do you only disagree with my first sentence or with both sentences?
Currently, I seem to be alone with my opinion (at least in qemu-devel). Nevertheless, the designers of C++ thought that casts from void * to T * were something very important. I don't know the history of their decision. I personally think that deriving a data type T from some bytes in memory which can contain anything is an operation which is worth being documented by the programmer, and this is exactly what the cast does.
My main reason why I try to write portable code is my personal experience with code reuse and the future development of compilers. It is quite possible that some day C compilers will require type casts like C++ compilers do today.
And even today I want to be able to share C code from QEMU with program code written in C++ (which makes a large part of my business work).
Anthony, there is already a mixture of coding styles in QEMU (also regarding type casts). This is not surprising for an open source project with many contributors. Do we really need additional regulations? I think the existing ones (those in CODING_STYLE) are very good, and they are sufficient. I'd appreciate it very much if all code in QEMU would respect this documented coding style. Today, it does not, and there was an agreement that we do not write commits which only change the coding style (at least for white space). I suggest to stick to this agreement for non white space coding style, too.
Let me give one more C/C++ example. Today, many data structures are declared like this: typedef struct T { ... } T; There is nothing wrong with it, but it can be improved in several ways:
* The declaration does not work with C++ (yes, I know that many programmers are not interested in C++ compatibility for QEMU).
* The declaration allows variable declarations using struct T var; or just T var; (which is the QEMU style). I think a declaration which does not enforce the correct style is less good.
* The declaration contains noise (bad argument, but many people like speaking of noise) :-)
Why don't we declare structures like this: typedef struct { ... } T;? I suggest this to be the new coding style for structure declarations because it is shorter, C++ compatible and unambiguous.
Kind regards
Stefan
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Hi,
Quoted 3 lines
Why don't we declare structures like this: typedef struct { ... } T;? I suggest this to be the new coding style for structure declarations because it is shorter, C++ compatible and unambiguous.
There are quite a few cases where this will simply not work. They usually use a slightly different declaration style though:
typedef struct T T;
struct T {
/* stuff here */
};
Reasons why this is used/needed:
(1) structs pointing to each other, like this:
typedef struct A A;
typedef struct B B;
struct A {
B *b;
};
struct B {
A *a;
};
(2) keep the struct private, let others pass around pointers to the struct in a typesafe way though:
header file:
typedef struct T T;
T* get_foo(args);
int do_something_with_foo(T*);
source file:
struct T {
/* stuff here */
};
cheers,
Gerd
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 8 lines · Gerd Hoffmann
Hi,
Quoted 3 lines
Why don't we declare structures like this: typedef struct { ... } T;? I suggest this to be the new coding style for structure declarations because it is shorter, C++ compatible and unambiguous.
There are quite a few cases where this will simply not work. They usually use a slightly different declaration style though:
...
Quoted 4 lines
(1) structs pointing to each other, like this:
typedef struct A A;
typedef struct B B;
You can use "typedef struct _A A" to be C++ compatible, but it fails to be shorter so I wouldn't recommend it ;-)
-- Jamie
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 9 lines · Jamie Lokier
Quoted 8 lines · Gerd Hoffmann
Hi,
Quoted 3 lines
Why don't we declare structures like this: typedef struct { ... } T;? I suggest this to be the new coding style for structure declarations because it is shorter, C++ compatible and unambiguous.
There are quite a few cases where this will simply not work. They usually use a slightly different declaration style though:
Quoted 8 lines
..
Quoted 4 lines
(1) structs pointing to each other, like this:
typedef struct A A;
typedef struct B B;You can use "typedef struct _A A" to be C++ compatible, but it fails to be shorter so I wouldn't recommend it ;-)
This is neither C nor C++ compatible, in fact it breaks both.
Signature · 1 line
mailto:[email protected]
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 9 lines · malc
Quoted 7 lines
Quoted 4 lines
(1) structs pointing to each other, like this:
typedef struct A A;
typedef struct B B;You can use "typedef struct _A A" to be C++ compatible, but it fails to be shorter so I wouldn't recommend it ;-)
This is neither C nor C++ compatible, in fact it breaks both.
You'll have to explain that statement.
It compiles fine for me in C and C++:
typedef struct _A A;
typedef struct _B B;
struct _A { struct _B *b; };
struct _B { struct _A *a; };
A *foo(B *arg);
Using this works equally well:
struct _A { B *b; };
struct _B { A *a; };
-- Jamie
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 6 lines · Jamie Lokier
Quoted 5 lines · malc
Quoted 5 lines
Quoted 4 lines
(1) structs pointing to each other, like this:
typedef struct A A;
typedef struct B B;
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
You can use "typedef struct _A A" to be C++ compatible, but it fails to be shorter so I wouldn't recommend it ;-)
This is neither C nor C++ compatible, in fact it breaks both.
You'll have to explain that statement.
ISO/IEC 9899:1999 7.1.3#1
and 17.4.3.1.2 of n2315 (Draft of c++0x)
mandate that those names are reserved.
[..snip..]
Signature · 1 line
mailto:[email protected]
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 10 lines · malc
Quoted 9 lines · Jamie Lokier
Quoted 8 lines · malc
Quoted 7 lines
Quoted 4 lines
(1) structs pointing to each other, like this:
typedef struct A A;
typedef struct B B;You can use "typedef struct _A A" to be C++ compatible, but it fails to be shorter so I wouldn't recommend it ;-)
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
This is neither C nor C++ compatible, in fact it breaks both.
You'll have to explain that statement.
ISO/IEC 9899:1999 7.1.3#1
and 17.4.3.1.2 of n2315 (Draft of c++0x)mandate that those names are reserved.
The _A is just an example; just substitute an identifier you're happy with. Use lower case, or put the underscore at the end, or something else.
It's actively unhelpful to say "that is not C/C++ compatible" without explaining that it's for an tangential reason which is easily avoided.
-- Jamie
Re: [Qemu-devel] Coding style, C++ compatible code
In reply to Stefan WeilQuoted 54 lines · Stefan Weil
Quoted 53 lines · Markus Armbruster
Quoted 38 lines · Stefan Weil
Quoted 37 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>---
hw/eepro100.c| 6 +++---1 files changed, 3 insertions(+), 3 deletions(-)hw/eepro100.c +3
-3@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)static void nic_reset (void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t sizestatic int nic_load (QEMUFile * f,void *opaque,int version_id) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;int ret;@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)static void nic_save (QEMUFile * f,void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;Quoted 4 lines
Quoted 2 lines
if (
s->pci_dev)I wrote these type casts, and I think they make sense. In C++ code, they are even mandatory.
Yes, but this isn't C++.
Quoted 5 lines
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
Quoted 7 lines
Quoted 3 lines
I respectfully disagree. Casts from "void *" to "T *" are pure noise. Getting into the habit of writing noise casts runs the risk of silencing warnings that flag real type errors.
Hello
Do you only disagree with my first sentence or with both sentences?
We can certainly discuss how to write better C, but appealing to the authority of the C++ rationale is an argument I don't buy.
I prefer not to attempt to write C code that behaves identically when interpreted as C++ (extern "C" seems safer and easier).
Does this answer your question?
Quoted 7 lines
Currently, I seem to be alone with my opinion (at least in qemu-devel). Nevertheless, the designers of C++ thought that casts from void * to T * were something very important. I don't know the history of their decision. I personally think that deriving a data type T from some bytes in memory which can contain anything is an operation which is worth being documented by the programmer, and this is exactly what the cast does.
I don't see much value in that, sorry.
Quoted 4 lines
My main reason why I try to write portable code is my personal experience with code reuse and the future development of compilers. It is quite possible that some day C compilers will require type casts like C++ compilers do today.
I can't read the ISO C committee's mind, but I believe such an incompatible change is extremely unlikely, given the pains they've taken not to break existing C code.
Quoted 3 lines
And even today I want to be able to share C code from QEMU with program code written in C++ (which makes a large part of my business work).
How about extern "C"?
[...]
Re: [Qemu-devel] Coding style, C++ compatible code
Quoted 5 lines · Markus Armbruster
Stefan Weil <[email protected]> writes:
We can certainly discuss how to write better C, but appealing to the authority of the C++ rationale is an argument I don't buy.
C++ compatibility is not a goal of QEMU AFAIK.
We make extensive use of the fact that you can use struct Foo or Foo as a declaration via the TAILQ_ENTRY macro. With your proposal, that macro would no longer work.
Regards,
Anthony Liguori
Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
In reply to Stefan WeilQuoted 58 lines · Stefan Weil
Quoted 50 lines · Markus Armbruster
Quoted 38 lines · Stefan Weil
Quoted 37 lines · Juan Quintela
Signed-off-by Juan Quintela <[email protected]>---
hw/eepro100.c| 6 +++---1 files changed, 3 insertions(+), 3 deletions(-)hw/eepro100.c +3
-3@@ -507,7 +507,7 @@ static void nic_selective_reset(EEPRO100State * s)static void nic_reset (void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;logout ("%p\n" , s);static int first;if (!first) {@@ -1544,7 +1544,7 @@ static ssize_t nic_receive(VLANClientState *vc, const uint8_t * buf, size_t sizestatic int nic_load (QEMUFile * f,void *opaque,int version_id) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;int ret;@@ -1634,7 +1634,7 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)static void nic_save (QEMUFile * f,void *opaque) { -EEPRO100State *s = (EEPRO100State *) opaque; +EEPRO100State *s = opaque;int i;Quoted 4 lines
Quoted 2 lines
if (
s->pci_dev)I wrote these type casts, and I think they make sense. In C++ code, they are even mandatory.
Yes, but this isn't C++.
Quoted 3 lines
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
Quoted 6 lines
Quoted 2 lines
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
I respectfully disagree. Casts from "void *" to "T *" are pure noise. Getting into the habit of writing noise casts runs the risk of silencing warnings that flag real type errors.
Quoted 8 lines
Hello
Do you only disagree with my first sentence or with both sentences?
Currently, I seem to be alone with my opinion (at least in qemu-devel). Nevertheless, the designers of C++ thought that casts from void * to T * were something very important. I don't know the history of their decision.
I don't know the history of C++ either. What I do know is that the void* type was added to the C language precisely to *avoid* such casts. The original K&R C used char* as return type for malloc() etc, and this of course required a cast.
Quoted 4 lines
I personally think that deriving a data type T from some bytes in memory which can contain anything is an operation which is worth being documented by the programmer, and this is exactly what the cast does.
The declaration and assignment already make that perfectly clear. The cast is at best noise, and often hides a real error.
Quoted 4 lines
My main reason why I try to write portable code is my personal experience with code reuse and the future development of compilers. It is quite possible that some day C compilers will require type casts like C++ compilers do today.
Any syntax or semantics can potentially change in the future. If *I* were in charge, I'd make frivolous casts an *error*.
Quoted 3 lines
And even today I want to be able to share C code from QEMU with program code written in C++ (which makes a large part of my business work).
That's your problem. Please don't bring it here.
Quoted 7 lines
Let me give one more C/C++ example. Today, many data structures are declared like this: typedef struct T { ... } T; There is nothing wrong with it, but it can be improved in several ways:
* The declaration does not work with C++ (yes, I know that many programmers are not interested in C++ compatibility for QEMU).
Nor does it work in Fortran or COBOL.
Quoted 10 lines
* The declaration allows variable declarations using struct T var; or just T var; (which is the QEMU style). I think a declaration which does not enforce the correct style is less good.
* The declaration contains noise (bad argument, but many people like speaking of noise) :-)
Why don't we declare structures like this: typedef struct { ... } T;? I suggest this to be the new coding style for structure declarations because it is shorter, C++ compatible and unambiguous.
In my personal projects, I never use typedefs for structs. Typing those 5 characters isn't much of a burden, and it makes it unambiguously clear that something is a struct and not something else. It even works in C++, should you care.
C is not C++, even though much of the syntax is confusingly similar. Trying to shoehorn the same coding style onto both is a mistake, and does more harm than good whichever of the languages you are writing in.
Signature · 2 lines
Måns Rullgård [email protected]
Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 8 lines · Måns Rullgård
Quoted 4 lines · Stefan Weil
I personally think that deriving a data type T from some bytes in memory which can contain anything is an operation which is worth being documented by the programmer, and this is exactly what the cast does.
The declaration and assignment already make that perfectly clear. The cast is at best noise, and often hides a real error.
There are additional points, having all those casts makes people used to them and generally makes it much harder to spot those that are just wrong (not that C++ does not have this issue, since basically all conversions/uses of void* are wrong, with C they are unavoidable). Having casts for malloc results also makes it needlessly hard to change the type. Use
var = malloc(count * sizeof(*var))
and you only need to change the declaration, add a cast and you also have to change all places where such allocations are done.
Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 11 lines · Reimar Döffinger
There are additional points, having all those casts makes people used to them and generally makes it much harder to spot those that are just wrong (not that C++ does not have this issue, since basically all conversions/uses of void* are wrong, with C they are unavoidable). Having casts for malloc results also makes it needlessly hard to change the type. Use
var =malloc(count * sizeof(*var))
and you only need to change the declaration, add a cast and you also have to change all places where such allocations are done.
Or better, NEW() and NEW_ARRAY().
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 1 line · Avi Kivity
Or better,
NEW()andNEW_ARRAY().
ISTR this being discussed before, but there was some disagreement regarding whether it is preferable to have:
QEMU_NEW(ptr);
or:
ptr = QEMU_NEW(type);
If the type is incorrect, the latter form would still at least yield a warning (and now therefore a build failure). It seems slightly more readable to me, so that's the form that I would have preferred...
Is there any reason that this wouldn't be accepted, or should I start submitting patches?
Cheers,
Signature · 1 line
Stuart Brady
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
In reply to Stefan WeilQuoted 16 lines · Stefan Weil
Quoted 9 lines
Quoted 3 lines
I wrote these type casts, and I think they make sense. In C++ code, they are even mandatory.
Yes, but this isn't C++.
Quoted 3 lines
I think the arguments why C++ requires this kind of type casts apply to C code (like in Qemu) as well.
Quoted 7 lines
Quoted 3 lines
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
I respectfully disagree. Casts from "void *" to "T *" are pure noise. Getting into the habit of writing noise casts runs the risk of silencing warnings that flag real type errors.
Quoted 6 lines
Hello
Do you only disagree with my first sentence or with both sentences?
Currently, I seem to be alone with my opinion (at least in qemu-devel).
You are not alone.
Quoted 7 lines
Nevertheless, the designers of C++ thought that casts from void * to T * were something very important. I don't know the history of their decision. I personally think that deriving a data type T from some bytes in memory which can contain anything is an operation which is worth being documented by the programmer, and this is exactly what the cast does.
Yes.
Quoted 8 lines
Let me give one more C/C++ example. Today, many data structures are declared like this: typedef struct T { ... } T; There is nothing wrong with it, but it can be improved in several ways:
* The declaration does not work with C++ (yes, I know that many programmers are not interested in C++ compatibility for QEMU).
It does work in C++.
Quoted 4 lines
* The declaration allows variable declarations using struct T var; or just T var; (which is the QEMU style). I think a declaration which does not enforce the correct style is less good.
It's often needed, for example to have a struct T * in T.
Signature · 1 line
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] Coding style, C++ compatible code (was Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense)
Quoted 9 lines · Avi Kivity
Quoted 6 lines · Stefan Weil
Nevertheless, the designers of C++ thought that casts from void * to T * were something very important. I don't know the history of their decision. I personally think that deriving a data type T from some bytes in memory which can contain anything is an operation which is worth being documented by the programmer, and this is exactly what the cast does.
Yes.
Not really. Adding the cast can hide bugs.
If you omit the cast, a C compiler will at least warn, and maybe error, if the original pointer is not "void *", or is "const void *" and you are removing the constness.
With the cast, the C compiler will silently let you compile buggy code.
In C++, results vary. You are supposed to use a base class pointer, and if you need a cast, you are supposed to use "static_cast<>" anyway.
In C++, it's an error without the cast, but it can also be an error with the cast:
g++ -Wall test.cc -c -Werror=old-style-cast
test.cc: In function ‘S* foo(void*)’:
test.cc:5: error: use of old-style cast
struct S *foo(void *ptr)
{
return (struct S *)ptr;
}
Of course there are dirty tricks to let you omit the "void *" pointer altogether. Let's not go there :-)
-- Jamie
Re: [Qemu-devel] [PATCH 02/22] eepro100: cast a void * makes no sense
In reply to Stefan WeilQuoted 6 lines
Review
Quoted 4 lines
static void nic_save(QEMUFile * f, void *opaque) {(quoted diff) +1
-1- EEPRO100State *s = (EEPRO100State *) opaque; + EEPRO100State *s = opaque;I wrote these type casts, and I think they make sense.
Why?
There is no technical reason for it.
And in that case it IMHO doesn't make sense to keep the cast as documentation.
Quoted 2 lines
If it is possible with no or very litte efforts to write code which is C and C++ compatible, I prefer to do so.
Point being? I doubt qemu will go C++ ...
cheers,
Gerd
PATCH 3/22 eepro100: Remove unused indirection of PCIDevice
In reply to Juan QuintelaOnce there, there is no way that we don't have a PCI Device at save/load time. Remove the check
--- hw/eepro100.c | 57 +++++++++++++++++++++++---------------------------------- 1 files changed, 23 insertions(+), 34 deletions(-)
hw/eepro100.c +23 -34
@@ -181,6 +181,7 @@ typedef enum { } ru_state_t; typedef struct { + PCIDevice dev; #if 1 uint8_t cmd; uint32_t start;
@@ -200,7 +201,6 @@ typedef struct {uint8_t curpag;uint8_t mult[8 ];/* multicast mask array */ int mmio_index; -PCIDevice *pci_dev;VLANClientState *vc;#endif uint8_t scb_stat;/* SCB stat/ack byte */
@@ -304,7 +304,7 @@ static void disable_interrupt(EEPRO100State * s) {if (s->int_stat) {logout ("interrupt disabled\n" ); -qemu_irq_lower (s->pci_dev->irq[0 ]); +qemu_irq_lower (s->dev.irq[0 ]); s->int_stat =0 ; } }
@@ -313,7 +313,7 @@ static void enable_interrupt(EEPRO100State * s) {if (!s->int_stat) {logout ("interrupt enabled\n" ); -qemu_irq_raise (s->pci_dev->irq[0 ]); +qemu_irq_raise (s->dev.irq[0 ]); s->int_stat =1 ; } }
@@ -392,7 +392,7 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)static void pci_reset (EEPRO100State * s) {uint32_t device = s->device; -uint8_t *pci_conf = s->pci_dev->config; +uint8_t *pci_conf = s->dev.config;logout ("%p\n" , s);
@@ -1338,16 +1338,10 @@ static void ioport_write4(void *opaque, uint32_t addr, uint32_t val)/***********************************************************/ /* PCI EEPRO100 definitions */ -typedef struct PCIEEPRO100State { -PCIDevice dev; -EEPRO100State eepro100; -}PCIEEPRO100State ; -static void pci_map (PCIDevice * pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev); -EEPRO100State *s = &d->eepro100; +EEPRO100State *s =DO_UPCAST (EEPRO100State, dev, pci_dev);logout ("region %d, addr=0x%08x, size=0x%08x, type=%d\n" , region_num, addr, size, type);
@@ -1420,15 +1414,15 @@ static CPUReadMemoryFunc *pci_mmio_read[] = {static void pci_mmio_map (PCIDevice * pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev); +EEPRO100State *s =DO_UPCAST (EEPRO100State, dev, pci_dev);logout ("region %d, addr=0x%08x, size=0x%08x, type=%d\n" , region_num, addr, size, type);if (region_num ==0 ) {/* Map control / status registers. */ -cpu_register_physical_memory (addr, size, d->eepro100.mmio_index); - d->eepro100.region[region_num] = addr; +cpu_register_physical_memory (addr, size, s->mmio_index); + s->region[region_num] = addr; } }
@@ -1551,8 +1545,8 @@ static int nic_load(QEMUFile * f, void *opaque, int version_id)if (version_id >3 )return -EINVAL ; -if (s->pci_dev && version_id >=3 ) { - ret =pci_device_load (s->pci_dev, f); +if (version_id >=3 ) { + ret =pci_device_load (&s->dev, f);if (ret <0 )return ret; }
@@ -1637,8 +1631,7 @@ static void nic_save(QEMUFile * f, void *opaque)EEPRO100State *s = opaque;int i; -if (s->pci_dev) -pci_device_save (s->pci_dev, f); +pci_device_save (&s->dev, f);qemu_put_8s (f, &s->rxcr);
@@ -1720,8 +1713,7 @@ static void nic_cleanup(VLANClientState *vc)static int pci_nic_uninit (PCIDevice *dev) { -PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, dev); -EEPRO100State *s = &d->eepro100; +EEPRO100State *s =DO_UPCAST (EEPRO100State, dev, dev);cpu_unregister_io_memory (s->mmio_index);
@@ -1730,16 +1722,13 @@ static int pci_nic_uninit(PCIDevice *dev)static void nic_init (PCIDevice *pci_dev,uint32_t device) { -PCIEEPRO100State *d =DO_UPCAST (PCIEEPRO100State, dev, pci_dev); -EEPRO100State *s; +EEPRO100State *s =DO_UPCAST (EEPRO100State, dev, pci_dev);logout ("\n" ); - d->dev.unregister = pci_nic_uninit; + s->dev.unregister = pci_nic_uninit; - s = &d->eepro100; s->device = device; - s->pci_dev = &d->dev;pci_reset (s);
@@ -1748,24 +1737,24 @@ static void nic_init(PCIDevice *pci_dev, uint32_t device) s->eeprom =eeprom93xx_new (EEPROM_SIZE );/* Handler for memory-mapped I/O */ - d->eepro100.mmio_index = + s->mmio_index =cpu_register_io_memory (pci_mmio_read, pci_mmio_write, s); -pci_register_bar (&d->dev,0 ,PCI_MEM_SIZE , +pci_register_bar (&s->dev,0 ,PCI_MEM_SIZE ,PCI_ADDRESS_SPACE_MEM |PCI_ADDRESS_SPACE_MEM_PREFETCH , pci_mmio_map); -pci_register_bar (&d->dev,1 ,PCI_IO_SIZE ,PCI_ADDRESS_SPACE_IO , +pci_register_bar (&s->dev,1 ,PCI_IO_SIZE ,PCI_ADDRESS_SPACE_IO , pci_map); -pci_register_bar (&d->dev,2 ,PCI_FLASH_SIZE ,PCI_ADDRESS_SPACE_MEM , +pci_register_bar (&s->dev,2 ,PCI_FLASH_SIZE ,PCI_ADDRESS_SPACE_MEM , pci_mmio_map); -qdev_get_macaddr (&d->dev.qdev, s->macaddr); +qdev_get_macaddr (&s->dev.qdev, s->macaddr);logout ("macaddr: %s\n" ,nic_dump (&s->macaddr[0 ],6 ));assert (s->region[1 ] ==0 );nic_reset (s); - s->vc =qdev_get_vlan_client (&d->dev.qdev, + s->vc =qdev_get_vlan_client (&s->dev.qdev, nic_can_receive, nic_receive,NULL , nic_cleanup, s);
@@ -1794,15 +1783,15 @@ static void pci_i82559er_init(PCIDevice *dev)static PCIDeviceInfo eepro100_info[] = { { .qdev.name ="i82551" , - .qdev.size =sizeof (PCIEEPRO100State), + .qdev.size =sizeof (EEPRO100State), .init = pci_i82551_init, },{ .qdev.name ="i82557b" , - .qdev.size =sizeof (PCIEEPRO100State), + .qdev.size =sizeof (EEPRO100State), .init = pci_i82557b_init, },{ .qdev.name ="i82559er" , - .qdev.size =sizeof (PCIEEPRO100State), + .qdev.size =sizeof (EEPRO100State), .init = pci_i82559er_init, },{/* end of list */
Signature · 1 line
1.6.2.5
PATCH 4/22 es1370: Remove unused indirection of PCIES1370State and ES1370State
In reply to Juan Quintela--- hw/es1370.c | 31 ++++++++++--------------------- 1 files changed, 10 insertions(+), 21 deletions(-)
hw/es1370.c +10 -21
@@ -266,8 +266,7 @@ struct chan { }; typedef struct ES1370State { - PCIDevice *pci_dev; - + PCIDevice dev; QEMUSoundCard card; struct chan chan[NB_CHANNELS]; SWVoiceOut *dac_voice[2];
@@ -280,11 +279,6 @@ typedef struct ES1370State {uint32_t sctl; } ES1370State; -typedef struct PCIES1370State { -PCIDevice dev; -ES1370State es1370; -}PCIES1370State ; -struct chan_bits {uint32_t ctl_en;uint32_t stat_int;
@@ -327,7 +321,7 @@ static void es1370_update_status (ES1370State *s, uint32_t new_status)else { s->status = new_status & ~STAT_INTR ; } -qemu_set_irq (s->pci_dev->irq[0 ], !!level); +qemu_set_irq (s->dev.irq[0 ], !!level); }static void es1370_reset (ES1370State *s)
@@ -353,7 +347,7 @@ static void es1370_reset (ES1370State *s) s->dac_voice[i] =NULL ; } } -qemu_irq_lower (s->pci_dev->irq[0 ]); +qemu_irq_lower (s->dev.irq[0 ]); }static void es1370_maybe_lower_irq (ES1370State *s,uint32_t sctl)
@@ -915,8 +909,7 @@ static void es1370_adc_callback (void *opaque, int avail)static void es1370_map (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIES1370State *d = (PCIES1370State *) pci_dev; -ES1370State *s = &d->es1370; +ES1370State *s =DO_UPCAST (ES1370State, dev, pci_dev); (void ) region_num; (void ) size;
@@ -936,7 +929,7 @@ static void es1370_save (QEMUFile *f, void *opaque)ES1370State *s = opaque;size_t i; -pci_device_save (s->pci_dev, f); +pci_device_save (&s->dev, f);for (i =0 ; i <NB_CHANNELS ; ++i) {struct chan *d = &s->chan[i];qemu_put_be32s (f, &d->shift);
@@ -962,7 +955,7 @@ static int es1370_load (QEMUFile *f, void *opaque, int version_id)if (version_id !=2 )return -EINVAL ; - ret =pci_device_load (s->pci_dev, f); + ret =pci_device_load (&s->dev, f);if (ret)return ret;
@@ -1007,9 +1000,8 @@ static void es1370_on_reset (void *opaque)static void es1370_initfn (PCIDevice *dev) { -PCIES1370State *d =DO_UPCAST (PCIES1370State, dev, dev); -ES1370State *s = &d->es1370; -uint8_t *c = d->dev.config; +ES1370State *s =DO_UPCAST (ES1370State, dev, dev); +uint8_t *c = s->dev.config;pci_config_set_vendor_id (c,PCI_VENDOR_ID_ENSONIQ );pci_config_set_device_id (c,PCI_DEVICE_ID_ENSONIQ_ES1370 );
@@ -1035,10 +1027,7 @@ static void es1370_initfn(PCIDevice *dev) c[0x3e ] =0x0c ; c[0x3f ] =0x80 ; - s = &d->es1370; - s->pci_dev = &d->dev; - -pci_register_bar (&d->dev,0 ,256 ,PCI_ADDRESS_SPACE_IO , es1370_map); +pci_register_bar (&s->dev,0 ,256 ,PCI_ADDRESS_SPACE_IO , es1370_map);register_savevm ("es1370" ,0 ,2 , es1370_save, es1370_load, s);qemu_register_reset (es1370_on_reset, s);
@@ -1055,7 +1044,7 @@ int es1370_init (PCIBus *bus)static PCIDeviceInfo es1370_info = { .qdev.name ="ES1370" , .qdev.desc ="ENSONIQ AudioPCI ES1370" , - .qdev.size =sizeof (PCIES1370State), + .qdev.size =sizeof (ES1370State), .init = es1370_initfn, };
Signature · 1 line
1.6.2.5
PATCH 5/22 ne2000: remove casts from void *
In reply to Juan Quintela--- hw/ne2000.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
hw/ne2000.c +2 -2
@@ -650,7 +650,7 @@ static uint32_t ne2000_reset_ioport_read(void *opaque, uint32_t addr)static void ne2000_save (QEMUFile * f,void * opaque) { -NE2000State * s=(NE2000State *)opaque; +NE2000State * s = opaque;uint32_t tmp;if (s->pci_dev)
@@ -681,7 +681,7 @@ static void ne2000_save(QEMUFile* f,void* opaque)static int ne2000_load (QEMUFile * f,void * opaque,int version_id) { -NE2000State * s=(NE2000State *)opaque; +NE2000State * s = opaque;int ret;uint32_t tmp;
Signature · 1 line
1.6.2.5
PATCH 6/22 ne2000: pci_dev has this very value with the right type
In reply to Juan Quintela--- hw/ne2000.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
hw/ne2000.c +1 -1
@@ -820,7 +820,7 @@ static void pci_ne2000_init(PCIDevice *pci_dev) PCI_ADDRESS_SPACE_IO, ne2000_map); s = &d->ne2000; s->irq = d->dev.irq[0 ]; - s->pci_dev = (PCIDevice *)d; + s->pci_dev = pci_dev;qdev_get_macaddr (&d->dev.qdev, s->macaddr);ne2000_reset (s); s->vc = qdev_get_vlan_client(&d->dev.qdev,
Signature · 1 line
1.6.2.5
PATCH 7/22 ne2000: Remove unneeded double indirection of PCINE2000State
In reply to Juan QuintelaOnce there, do the right DO_UPCAST instead of cast.
Note that we maintain pci_dev field in this case, because there exist ne2000 isa cards. They to diferentiate them is that isa ones don't set this link.
--- hw/ne2000.c | 27 ++++++++++----------------- 1 files changed, 10 insertions(+), 17 deletions(-)
hw/ne2000.c +10 -17
@@ -122,6 +122,7 @@#define NE2000_MEM_SIZE NE2000_PMEM_END typedef struct NE2000State { + PCIDevice dev; uint8_t cmd; uint32_t start; uint32_t stop;
@@ -771,16 +772,10 @@ void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd)/***********************************************************/ /* PCI NE2000 definitions */ -typedef struct PCINE2000State { -PCIDevice dev; -NE2000State ne2000; -}PCINE2000State ; -static void ne2000_map (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCINE2000State *d = (PCINE2000State *)pci_dev; -NE2000State *s = &d->ne2000; +NE2000State *s =DO_UPCAST (NE2000State, dev, pci_dev);register_ioport_write (addr,16 ,1 , ne2000_ioport_write, s);register_ioport_read (addr,16 ,1 , ne2000_ioport_read, s);
@@ -805,25 +800,23 @@ static void ne2000_cleanup(VLANClientState *vc)static void pci_ne2000_init (PCIDevice *pci_dev) { -PCINE2000State *d = (PCINE2000State *)pci_dev; -NE2000State *s; +NE2000State *s =DO_UPCAST (NE2000State, dev, pci_dev);uint8_t *pci_conf; - pci_conf = d->dev.config; + pci_conf = s->dev.config;pci_config_set_vendor_id (pci_conf,PCI_VENDOR_ID_REALTEK );pci_config_set_device_id (pci_conf,PCI_DEVICE_ID_REALTEK_8029 );pci_config_set_class (pci_conf,PCI_CLASS_NETWORK_ETHERNET ); pci_conf[PCI_HEADER_TYPE ] =PCI_HEADER_TYPE_NORMAL ;// header_type pci_conf[0x3d ] =1 ;// interrupt pin 0 -pci_register_bar (&d->dev,0 ,0x100 , +pci_register_bar (&s->dev,0 ,0x100 ,PCI_ADDRESS_SPACE_IO , ne2000_map); - s = &d->ne2000; - s->irq = d->dev.irq[0 ]; - s->pci_dev = pci_dev; -qdev_get_macaddr (&d->dev.qdev, s->macaddr); + s->irq = s->dev.irq[0 ]; + s->pci_dev = &s->dev; +qdev_get_macaddr (&s->dev.qdev, s->macaddr);ne2000_reset (s); - s->vc =qdev_get_vlan_client (&d->dev.qdev, + s->vc =qdev_get_vlan_client (&s->dev.qdev, ne2000_can_receive, ne2000_receive,NULL , ne2000_cleanup, s);
@@ -834,7 +827,7 @@ static void pci_ne2000_init(PCIDevice *pci_dev)static PCIDeviceInfo ne2000_info = { .qdev.name ="ne2k_pci" , - .qdev.size =sizeof (PCINE2000State), + .qdev.size =sizeof (NE2000State), .init = pci_ne2000_init, };
Signature · 1 line
1.6.2.5
Gerd Hoffmann
NAKRe: [Qemu-devel] [PATCH 07/22] ne2000: Remove unneeded double indirection of PCINE2000State
hw/ne2000.c void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd) · line 777
Quoted 8 lines
typedef struct NE2000State {hw/ne2000.c +1
-4+PCIDevice dev;uint8_t cmd; -typedef struct PCINE2000State { -PCIDevice dev; -NE2000State ne2000; -}PCINE2000State ;
NACK.
We'll have a ...
typedef struct ISANE2000State {
ISADevice dev;
NE2000State ne2000;
} ISANE2000State;
... some day.
cheers,
Gerd
[Qemu-devel] Re: [PATCH 07/22] ne2000: Remove unneeded double indirection of PCINE2000State
Quoted 11 lines · Gerd Hoffmann
hw/ne2000.c void isa_ne2000_init(int base, qemu_irq irq, NICInfo *nd) · line 777
Quoted 8 lines
typedef struct NE2000State {hw/ne2000.c +1
-4+PCIDevice dev;uint8_t cmd; -typedef struct PCINE2000State { -PCIDevice dev; -NE2000State ne2000; -}PCINE2000State ;NACK.
Quoted 8 lines
We'll have a ...
typedef struct ISANE2000State { ISADevice dev; NE2000State ne2000; } ISANE2000State;... some day.
Ok, redoing it this way.
Thanks for the review.
Later, Juan.
PATCH 8/22 ne2000: change pci_dev to is_pci
In reply to Juan Quintela--- hw/ne2000.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
hw/ne2000.c +6 -6
@@ -142,7 +142,7 @@ typedef struct NE2000State {uint8_t mult[8 ];/* multicast mask array */ qemu_irq irq;int isa_io_base; -PCIDevice *pci_dev; +int is_pci;VLANClientState *vc;uint8_t macaddr[6 ];uint8_t mem[NE2000_MEM_SIZE ];
@@ -654,8 +654,8 @@ static void ne2000_save(QEMUFile* f,void* opaque)NE2000State * s = opaque;uint32_t tmp; -if (s->pci_dev) -pci_device_save (s->pci_dev, f); +if (s->is_pci) +pci_device_save (&s->dev, f);qemu_put_8s (f, &s->rxcr);
@@ -689,8 +689,8 @@ static int ne2000_load(QEMUFile* f,void* opaque,int version_id)if (version_id >3 )return -EINVAL ; -if (s->pci_dev && version_id >=3 ) { - ret =pci_device_load (s->pci_dev, f); +if (s->is_pci && version_id >=3 ) { + ret =pci_device_load (&s->dev, f);if (ret <0 )return ret; }
@@ -813,7 +813,7 @@ static void pci_ne2000_init(PCIDevice *pci_dev)pci_register_bar (&s->dev,0 ,0x100 ,PCI_ADDRESS_SPACE_IO , ne2000_map); s->irq = s->dev.irq[0 ]; - s->pci_dev = &s->dev; + s->is_pci =1 ;qdev_get_macaddr (&s->dev.qdev, s->macaddr);ne2000_reset (s); s->vc = qdev_get_vlan_client(&s->dev.qdev,
Signature · 1 line
1.6.2.5
PATCH 9/22 pci: remove casts from void *
In reply to Juan Quintela--- hw/pci.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
hw/pci.c +4 -4
@@ -75,7 +75,7 @@ static PCIBus *first_bus;static void pcibus_save (QEMUFile *f,void *opaque) { -PCIBus *bus = (PCIBus *)opaque; +PCIBus *bus = opaque;int i;qemu_put_be32 (f, bus->nirq);
@@ -85,7 +85,7 @@ static void pcibus_save(QEMUFile *f, void *opaque)static int pcibus_load (QEMUFile *f,void *opaque,int version_id) { -PCIBus *bus = (PCIBus *)opaque; +PCIBus *bus = opaque;int i, nirq;if (version_id !=1 )
@@ -106,7 +106,7 @@ static int pcibus_load(QEMUFile *f, void *opaque, int version_id)static void pci_bus_reset (void *opaque) { -PCIBus *bus = (PCIBus *)opaque; +PCIBus *bus = opaque;int i;for (i =0 ; i < bus->nirq; i++) {
@@ -624,7 +624,7 @@ uint32_t pci_data_read(void *opaque, uint32_t addr, int len)/* 0 <= irq_num <= 3. level must be 0 or 1 */ static void pci_set_irq (void *opaque,int irq_num,int level) { -PCIDevice *pci_dev = (PCIDevice *)opaque; +PCIDevice *pci_dev = opaque;PCIBus *bus;int change;
Signature · 1 line
1.6.2.5
PATCH 10/22 rtl8139: Remove unneeded double indirection of PCIRTL8139State
In reply to Juan Quintela--- hw/rtl8139.c | 38 +++++++++++++------------------------- 1 files changed, 13 insertions(+), 25 deletions(-)
hw/rtl8139.c +13 -25
@@ -423,6 +423,7 @@ static void RTL8139TallyCounters_load(QEMUFile* f, RTL8139TallyCounters *tally_cstatic void RTL8139TallyCounters_save (QEMUFile * f,RTL8139TallyCounters *tally_counters); typedef struct RTL8139State { + PCIDevice dev; uint8_t phys[8]; /* mac address */ uint8_t mult[8]; /* multicast mask array */
@@ -463,7 +464,6 @@ typedef struct RTL8139State {uint16_t CpCmd;uint8_t TxThresh; -PCIDevice *pci_dev;VLANClientState *vc;uint8_t macaddr[6 ];int rtl8139_mmio_io_addr;
@@ -692,7 +692,7 @@ static void rtl8139_update_irq(RTL8139State *s)DEBUG_PRINT (("RTL8139: Set IRQ to %d (%04x %04x)\n" , isr ?1 :0 , s->IntrStatus, s->IntrMask)); -qemu_set_irq (s->pci_dev->irq[0 ], (isr !=0 )); +qemu_set_irq (s->dev.irq[0 ], (isr !=0 )); }#define POLYNOMIAL 0x04c11db6
@@ -3121,7 +3121,7 @@ static void rtl8139_save(QEMUFile* f,void* opaque)RTL8139State * s=(RTL8139State *)opaque;unsigned int i; -pci_device_save (s->pci_dev, f); +pci_device_save (&s->dev, f);qemu_put_buffer (f, s->phys,6 );qemu_put_buffer (f, s->mult,8 );
@@ -3215,7 +3215,7 @@ static int rtl8139_load(QEMUFile* f,void* opaque,int version_id)return -EINVAL ;if (version_id >=3 ) { - ret =pci_device_load (s->pci_dev, f); + ret =pci_device_load (&s->dev, f);if (ret <0 )return ret; }
@@ -3323,16 +3323,10 @@ static int rtl8139_load(QEMUFile* f,void* opaque,int version_id)/***********************************************************/ /* PCI RTL8139 definitions */ -typedef struct PCIRTL8139State { -PCIDevice dev; -RTL8139State rtl8139; -}PCIRTL8139State ; -static void rtl8139_mmio_map (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIRTL8139State *d = (PCIRTL8139State *)pci_dev; -RTL8139State *s = &d->rtl8139; +RTL8139State *s =DO_UPCAST (RTL8139State, dev, pci_dev);cpu_register_physical_memory (addr +0 ,0x100 , s->rtl8139_mmio_io_addr); }
@@ -3340,8 +3334,7 @@ static void rtl8139_mmio_map(PCIDevice *pci_dev, int region_num,static void rtl8139_ioport_map (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -PCIRTL8139State *d = (PCIRTL8139State *)pci_dev; -RTL8139State *s = &d->rtl8139; +RTL8139State *s =DO_UPCAST (RTL8139State, dev, pci_dev);register_ioport_write (addr,0x100 ,1 , rtl8139_ioport_writeb, s);register_ioport_read ( addr,0x100 ,1 , rtl8139_ioport_readb, s);
@@ -3437,8 +3430,7 @@ static void rtl8139_cleanup(VLANClientState *vc)static int pci_rtl8139_uninit (PCIDevice *dev) { -PCIRTL8139State *d = (PCIRTL8139State *)dev; -RTL8139State *s = &d->rtl8139; +RTL8139State *s =DO_UPCAST (RTL8139State, dev, dev);cpu_unregister_io_memory (s->rtl8139_mmio_io_addr);
@@ -3447,13 +3439,12 @@ static int pci_rtl8139_uninit(PCIDevice *dev)static void pci_rtl8139_init (PCIDevice *dev) { -PCIRTL8139State *d = (PCIRTL8139State *)dev; -RTL8139State *s; +RTL8139State * s =DO_UPCAST (RTL8139State, dev, dev);uint8_t *pci_conf; - d->dev.unregister = pci_rtl8139_uninit; + s->dev.unregister = pci_rtl8139_uninit; - pci_conf = d->dev.config; + pci_conf = s->dev.config;pci_config_set_vendor_id (pci_conf,PCI_VENDOR_ID_REALTEK );pci_config_set_device_id (pci_conf,PCI_DEVICE_ID_REALTEK_8139 ); pci_conf[0x04 ] =0x05 ;/* command = I/O space, Bus Master */
@@ -3463,19 +3454,16 @@ static void pci_rtl8139_init(PCIDevice *dev) pci_conf[0x3d ] =1 ;/* interrupt pin 0 */ pci_conf[0x34 ] =0xdc ; - s = &d->rtl8139; -/* I/O handler for memory-mapped I/O */ s->rtl8139_mmio_io_addr =cpu_register_io_memory (rtl8139_mmio_read, rtl8139_mmio_write, s); -pci_register_bar (&d->dev,0 ,0x100 , +pci_register_bar (&s->dev,0 ,0x100 ,PCI_ADDRESS_SPACE_IO , rtl8139_ioport_map); -pci_register_bar (&d->dev,1 ,0x100 , +pci_register_bar (&s->dev,1 ,0x100 ,PCI_ADDRESS_SPACE_MEM , rtl8139_mmio_map); - s->pci_dev = (PCIDevice *)d;qdev_get_macaddr (&dev->qdev, s->macaddr);qemu_register_reset (rtl8139_reset, s);rtl8139_reset (s);
@@ -3501,7 +3489,7 @@ static void pci_rtl8139_init(PCIDevice *dev)static PCIDeviceInfo rtl8139_info = { .qdev.name ="rtl8139" , - .qdev.size =sizeof (PCIRTL8139State), + .qdev.size =sizeof (RTL8139State), .init = pci_rtl8139_init, };
Signature · 1 line
1.6.2.5
PATCH 11/22 rtl8139: remove pointless cast from void *
In reply to Juan Quintela--- hw/rtl8139.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
hw/rtl8139.c +2 -2
@@ -3118,7 +3118,7 @@ static uint32_t rtl8139_mmio_readl(void *opaque, target_phys_addr_t addr)static void rtl8139_save (QEMUFile * f,void * opaque) { -RTL8139State * s=(RTL8139State *)opaque; +RTL8139State * s = opaque;unsigned int i;pci_device_save (&s->dev, f);
@@ -3206,7 +3206,7 @@ static void rtl8139_save(QEMUFile* f,void* opaque)static int rtl8139_load (QEMUFile * f,void * opaque,int version_id) { -RTL8139State * s=(RTL8139State *)opaque; +RTL8139State * s = opaque;unsigned int i;int ret;
Signature · 1 line
1.6.2.5
PATCH 12/22 lsi53c895a: remove pointless cast from void *
In reply to Juan Quintela--- hw/lsi53c895a.c | 38 +++++++++++++++++++------------------- 1 files changed, 19 insertions(+), 19 deletions(-)
hw/lsi53c895a.c +19 -19
@@ -635,7 +635,7 @@ static int lsi_queue_tag(LSIState *s, uint32_t tag, uint32_t arg)static void lsi_command_complete (void *opaque,int reason,uint32_t tag,uint32_t arg) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;int out; out = (s->sstat1 &PHASE_MASK ) ==PHASE_DO ;
@@ -1724,14 +1724,14 @@ static void lsi_reg_writeb(LSIState *s, int offset, uint8_t val)static void lsi_mmio_writeb (void *opaque,target_phys_addr_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;lsi_reg_writeb (s, addr &0xff , val); }static void lsi_mmio_writew (void *opaque,target_phys_addr_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque; addr &=0xff ;lsi_reg_writeb (s, addr, val &0xff );
@@ -1740,7 +1740,7 @@ static void lsi_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)static void lsi_mmio_writel (void *opaque,target_phys_addr_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque; addr &=0xff ;lsi_reg_writeb (s, addr, val &0xff );
@@ -1751,14 +1751,14 @@ static void lsi_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)static uint32_t lsi_mmio_readb (void *opaque,target_phys_addr_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;return lsi_reg_readb (s, addr &0xff ); }static uint32_t lsi_mmio_readw (void *opaque,target_phys_addr_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t val; addr &=0xff ;
@@ -1769,7 +1769,7 @@ static uint32_t lsi_mmio_readw(void *opaque, target_phys_addr_t addr)static uint32_t lsi_mmio_readl (void *opaque,target_phys_addr_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t val; addr &=0xff ; val =lsi_reg_readb (s, addr);
@@ -1793,7 +1793,7 @@ static CPUWriteMemoryFunc *lsi_mmio_writefn[3] = {static void lsi_ram_writeb (void *opaque,target_phys_addr_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t newval;int shift;
@@ -1807,7 +1807,7 @@ static void lsi_ram_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)static void lsi_ram_writew (void *opaque,target_phys_addr_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t newval; addr &=0x1fff ;
@@ -1823,7 +1823,7 @@ static void lsi_ram_writew(void *opaque, target_phys_addr_t addr, uint32_t val)static void lsi_ram_writel (void *opaque,target_phys_addr_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque; addr &=0x1fff ; s->script_ram[addr >>2 ] = val;
@@ -1831,7 +1831,7 @@ static void lsi_ram_writel(void *opaque, target_phys_addr_t addr, uint32_t val)static uint32_t lsi_ram_readb (void *opaque,target_phys_addr_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t val; addr &=0x1fff ;
@@ -1842,7 +1842,7 @@ static uint32_t lsi_ram_readb(void *opaque, target_phys_addr_t addr)static uint32_t lsi_ram_readw (void *opaque,target_phys_addr_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t val; addr &=0x1fff ;
@@ -1854,7 +1854,7 @@ static uint32_t lsi_ram_readw(void *opaque, target_phys_addr_t addr)static uint32_t lsi_ram_readl (void *opaque,target_phys_addr_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque; addr &=0x1fff ;return le32_to_cpu (s->script_ram[addr >>2 ]);
@@ -1874,13 +1874,13 @@ static CPUWriteMemoryFunc *lsi_ram_writefn[3] = {static uint32_t lsi_io_readb (void *opaque,uint32_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;return lsi_reg_readb (s, addr &0xff ); }static uint32_t lsi_io_readw (void *opaque,uint32_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t val; addr &=0xff ; val =lsi_reg_readb (s, addr);
@@ -1890,7 +1890,7 @@ static uint32_t lsi_io_readw(void *opaque, uint32_t addr)static uint32_t lsi_io_readl (void *opaque,uint32_t addr) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;uint32_t val; addr &=0xff ; val =lsi_reg_readb (s, addr);
@@ -1902,13 +1902,13 @@ static uint32_t lsi_io_readl(void *opaque, uint32_t addr)static void lsi_io_writeb (void *opaque,uint32_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque;lsi_reg_writeb (s, addr &0xff , val); }static void lsi_io_writew (void *opaque,uint32_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque; addr &=0xff ;lsi_reg_writeb (s, addr, val &0xff );lsi_reg_writeb (s, addr +1 , (val >>8 ) &0xff );
@@ -1916,7 +1916,7 @@ static void lsi_io_writew(void *opaque, uint32_t addr, uint32_t val)static void lsi_io_writel (void *opaque,uint32_t addr,uint32_t val) { -LSIState *s = (LSIState *)opaque; +LSIState *s = opaque; addr &=0xff ;lsi_reg_writeb (s, addr, val &0xff );lsi_reg_writeb (s, addr +1 , (val >>8 ) &0xff );
Signature · 1 line
1.6.2.5
PATCH 13/22 lsi53c895a: use DO_UPCAST to cast from PCIDevice
In reply to Juan Quintela--- hw/lsi53c895a.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
hw/lsi53c895a.c +5 -5
@@ -1927,7 +1927,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)static void lsi_io_mapfunc (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -LSIState *s = (LSIState *)pci_dev; +LSIState *s =DO_UPCAST (LSIState, pci_dev, pci_dev);DPRINTF ("Mapping IO at %08x\n" , addr);
@@ -1942,7 +1942,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,static void lsi_ram_mapfunc (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -LSIState *s = (LSIState *)pci_dev; +LSIState *s =DO_UPCAST (LSIState, pci_dev, pci_dev);DPRINTF ("Mapping ram at %08x\n" , addr); s->script_ram_base = addr;
@@ -1952,7 +1952,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,static void lsi_mmio_mapfunc (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -LSIState *s = (LSIState *)pci_dev; +LSIState *s =DO_UPCAST (LSIState, pci_dev, pci_dev);DPRINTF ("Mapping registers at %08x\n" , addr);cpu_register_physical_memory (addr +0 ,0x400 , s->mmio_io_addr);
@@ -2153,7 +2153,7 @@ static int lsi_scsi_load(QEMUFile *f, void *opaque, int version_id)static int lsi_scsi_uninit (PCIDevice *d) { -LSIState *s = (LSIState *) d; +LSIState *s =DO_UPCAST (LSIState, pci_dev, d);cpu_unregister_io_memory (s->mmio_io_addr);cpu_unregister_io_memory (s->ram_io_addr);
@@ -2165,7 +2165,7 @@ static int lsi_scsi_uninit(PCIDevice *d)static void lsi_scsi_init (PCIDevice *dev) { -LSIState *s = (LSIState *)dev; +LSIState *s =DO_UPCAST (LSIState, pci_dev, dev);uint8_t *pci_conf; pci_conf = s->pci_dev.config;
Signature · 1 line
1.6.2.5
PATCH 14/22 lsi53c895a: rename PCIDevice field from pci_dev to dev (consistence)
In reply to Juan Quintela--- hw/lsi53c895a.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-)
hw/lsi53c895a.c +12 -12
@@ -175,7 +175,7 @@ typedef struct { } lsi_queue; typedef struct { - PCIDevice pci_dev; + PCIDevice dev; int mmio_io_addr; int ram_io_addr; uint32_t script_ram_base;
@@ -410,7 +410,7 @@ static void lsi_update_irq(LSIState *s) level, s->dstat, s->sist1, s->sist0); last_level = level; } -qemu_set_irq (s->pci_dev.irq[0 ], level); +qemu_set_irq (s->dev.irq[0 ], level); }/* Stop SCRIPTS execution and raise a SCSI interrupt. */
@@ -1927,7 +1927,7 @@ static void lsi_io_writel(void *opaque, uint32_t addr, uint32_t val)static void lsi_io_mapfunc (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -LSIState *s =DO_UPCAST (LSIState, pci_dev, pci_dev); +LSIState *s =DO_UPCAST (LSIState, dev, pci_dev);DPRINTF ("Mapping IO at %08x\n" , addr);
@@ -1942,7 +1942,7 @@ static void lsi_io_mapfunc(PCIDevice *pci_dev, int region_num,static void lsi_ram_mapfunc (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -LSIState *s =DO_UPCAST (LSIState, pci_dev, pci_dev); +LSIState *s =DO_UPCAST (LSIState, dev, pci_dev);DPRINTF ("Mapping ram at %08x\n" , addr); s->script_ram_base = addr;
@@ -1952,7 +1952,7 @@ static void lsi_ram_mapfunc(PCIDevice *pci_dev, int region_num,static void lsi_mmio_mapfunc (PCIDevice *pci_dev,int region_num,uint32_t addr,uint32_t size,int type) { -LSIState *s =DO_UPCAST (LSIState, pci_dev, pci_dev); +LSIState *s =DO_UPCAST (LSIState, dev, pci_dev);DPRINTF ("Mapping registers at %08x\n" , addr);cpu_register_physical_memory (addr +0 ,0x400 , s->mmio_io_addr);
@@ -1980,7 +1980,7 @@ void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id) s->scsi_dev[id] =scsi_generic_init (bd,1 , lsi_command_complete, s);if (s->scsi_dev[id] ==NULL ) s->scsi_dev[id] =scsi_disk_init (bd,1 , lsi_command_complete, s); - bd->private = &s->pci_dev; + bd->private = &s->dev; }static void lsi_scsi_save (QEMUFile *f,void *opaque)
@@ -1991,7 +1991,7 @@ static void lsi_scsi_save(QEMUFile *f, void *opaque)assert (s->current_dma_len ==0 );assert (s->active_commands ==0 ); -pci_device_save (&s->pci_dev, f); +pci_device_save (&s->dev, f);qemu_put_sbe32s (f, &s->carry);qemu_put_sbe32s (f, &s->sense);
@@ -2074,7 +2074,7 @@ static int lsi_scsi_load(QEMUFile *f, void *opaque, int version_id)return -EINVAL ; } -if ((ret =pci_device_load (&s->pci_dev, f)) <0 ) +if ((ret =pci_device_load (&s->dev, f)) <0 )return ret;qemu_get_sbe32s (f, &s->carry);
@@ -2153,7 +2153,7 @@ static int lsi_scsi_load(QEMUFile *f, void *opaque, int version_id)static int lsi_scsi_uninit (PCIDevice *d) { -LSIState *s =DO_UPCAST (LSIState, pci_dev, d); +LSIState *s =DO_UPCAST (LSIState, dev, d);cpu_unregister_io_memory (s->mmio_io_addr);cpu_unregister_io_memory (s->ram_io_addr);
@@ -2165,10 +2165,10 @@ static int lsi_scsi_uninit(PCIDevice *d)static void lsi_scsi_init (PCIDevice *dev) { -LSIState *s =DO_UPCAST (LSIState, pci_dev, dev); +LSIState *s =DO_UPCAST (LSIState, dev, dev);uint8_t *pci_conf; - pci_conf = s->pci_dev.config; + pci_conf = s->dev.config;/* PCI Vendor ID (word) */ pci_config_set_vendor_id (pci_conf,PCI_VENDOR_ID_LSI_LOGIC );
@@ -2198,7 +2198,7 @@ static void lsi_scsi_init(PCIDevice *dev) s->queue =qemu_malloc (sizeof (lsi_queue)); s->queue_len =1 ; s->active_commands =0 ; - s->pci_dev.unregister = lsi_scsi_uninit; + s->dev.unregister = lsi_scsi_uninit;lsi_soft_reset (s);
Signature · 1 line
1.6.2.5
PATCH 15/22 lsi53c895a: LSIState is a PCIDevice is a DeviceHost
In reply to Juan QuintelaGo figure.
--- hw/lsi53c895a.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
hw/lsi53c895a.c +2 -1
@@ -1960,7 +1960,8 @@ static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num,void lsi_scsi_attach (DeviceState *host,BlockDriverState *bd,int id) { -LSIState *s = (LSIState *)host; +PCIDevice *d =DO_UPCAST (PCIDevice, qdev, host); +LSIState *s =DO_UPCAST (LSIState, dev, d);if (id <0 ) {for (id =0 ; id <LSI_MAX_DEVS ; id++) {
Signature · 1 line
1.6.2.5
Re: [Qemu-devel] [PATCH 15/22] lsi53c895a: LSIState is a PCIDevice is a DeviceHost
hw/lsi53c895a.c static void lsi_mmio_mapfunc(PCIDevice *pci_dev, int region_num, · line 1964
Quoted 5 lines
void lsi_scsi_attach(DeviceState *host, BlockDriverState *bd, int id) {hw/lsi53c895a.c +2
-1-LSIState *s = (LSIState *)host; +PCIDevice *d =DO_UPCAST (PCIDevice, qdev, host); +LSIState *s =DO_UPCAST (LSIState, dev, d);
LSIState *s = DO_UPCAST(LSIState, dev.qdev, host);
cheers,
Gerd
PATCH 16/22 usb-ohci: Remove unneeded double indirection of OHCIPCIState
In reply to Juan Quintela--- hw/usb-ohci.c | 34 ++++++++++++++-------------------- 1 files changed, 14 insertions(+), 20 deletions(-)
hw/usb-ohci.c +14 -20
@@ -65,6 +65,7 @@ enum ohci_type { }; typedef struct { + PCIDevice dev; qemu_irq irq; enum ohci_type type; int mem;
@@ -1698,41 +1699,34 @@ static void usb_ohci_init(OHCIState *ohci, int num_ports, int devfn,ohci_reset (ohci); } -typedef struct { -PCIDevice pci_dev; -OHCIState state; -}OHCIPCIState ; -static void ohci_mapfunc (PCIDevice *pci_dev,int i,uint32_t addr,uint32_t size,int type) { -OHCIPCIState *ohci = (OHCIPCIState *)pci_dev; -cpu_register_physical_memory (addr, size, ohci->state.mem); +OHCIState *ohci =DO_UPCAST (OHCIState, dev, pci_dev); +cpu_register_physical_memory (addr, size, ohci->mem); }void usb_ohci_init_pci (struct PCIBus *bus,int num_ports,int devfn) { -OHCIPCIState *ohci; +PCIDevice *pci_dev =pci_register_device (bus,"OHCI USB" ,sizeof (OHCIState), + devfn,NULL ,NULL ); +OHCIState *ohci =DO_UPCAST (OHCIState, dev, pci_dev); - ohci = (OHCIPCIState *)pci_register_device (bus,"OHCI USB" ,sizeof (*ohci), - devfn,NULL ,NULL );if (ohci ==NULL ) {fprintf (stderr,"usb-ohci: Failed to register PCI device\n" );return ; } -pci_config_set_vendor_id (ohci->pci_dev.config,PCI_VENDOR_ID_APPLE ); -pci_config_set_device_id (ohci->pci_dev.config, -PCI_DEVICE_ID_APPLE_IPID_USB ); - ohci->pci_dev.config[0x09 ] =0x10 ;/* OHCI */ -pci_config_set_class (ohci->pci_dev.config,PCI_CLASS_SERIAL_USB ); - ohci->pci_dev.config[0x3d ] =0x01 ;/* interrupt pin 1 */ +pci_config_set_vendor_id (ohci->dev.config,PCI_VENDOR_ID_APPLE ); +pci_config_set_device_id (ohci->dev.config,PCI_DEVICE_ID_APPLE_IPID_USB ); + ohci->dev.config[0x09 ] =0x10 ;/* OHCI */ +pci_config_set_class (ohci->dev.config,PCI_CLASS_SERIAL_USB ); + ohci->dev.config[0x3d ] =0x01 ;/* interrupt pin 1 */ -usb_ohci_init (&ohci->state, num_ports, devfn, ohci->pci_dev.irq[0 ], -OHCI_TYPE_PCI , ohci->pci_dev.name,0 ); +usb_ohci_init (ohci, num_ports, devfn, ohci->dev.irq[0 ], +OHCI_TYPE_PCI , ohci->dev.name,0 ); -pci_register_bar ((struct PCIDevice *)ohci,0 ,256 , -PCI_ADDRESS_SPACE_MEM , ohci_mapfunc); +pci_register_bar (pci_dev,0 ,256 ,PCI_ADDRESS_SPACE_MEM , ohci_mapfunc); } void usb_ohci_init_pxa(target_phys_addr_t base, int num_ports, int devfn,
Signature · 1 line
1.6.2.5
Re: [Qemu-devel] [PATCH 16/22] usb-ohci: Remove unneeded double indirection of OHCIPCIState
hw/usb-ohci.c
Quoted 2 lines
hw/usb-ohci.c +0
-0
NACK. There are non-pci OHCI users.
cheers,
Gerd
PATCH 17/22 cirrus_vga: Remove unneeded double indirection of PCICirrusVGAState
In reply to Juan Quintela--- hw/cirrus_vga.c | 30 ++++++++++++------------------ 1 files changed, 12 insertions(+), 18 deletions(-)
hw/cirrus_vga.c +12 -18
@@ -238,6 +238,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s, uint8_t *dst, int dst_pitch, int width, int height); typedef struct CirrusVGAState { + PCIDevice dev; VGACommonState vga; int cirrus_linear_io_addr;
@@ -282,11 +283,6 @@ typedef struct CirrusVGAState {int bustype; } CirrusVGAState; -typedef struct PCICirrusVGAState { -PCIDevice dev; -CirrusVGAState cirrus_vga; -}PCICirrusVGAState ; -static uint8_t rop_to_index[256 ];/***************************************
@@ -3125,7 +3121,7 @@ static void cirrus_reset(void *opaque) {CirrusVGAState *s = opaque; -vga_reset (s); +vga_reset (&s->vga);unmap_linear_vram (s); s->vga.sr[0x06 ] =0x0f ;if (s->device_id ==CIRRUS_ID_CLGD5446 ) {
@@ -3263,7 +3259,7 @@ void isa_cirrus_vga_init(void)static void cirrus_pci_lfb_map (PCIDevice *d,int region_num,uint32_t addr,uint32_t size,int type) { -CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga; +CirrusVGAState *s =DO_UPCAST (CirrusVGAState, dev, d);/* XXX: add byte swapping apertures */ cpu_register_physical_memory(addr, s->vga.vram_size,
@@ -3284,7 +3280,7 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,static void cirrus_pci_mmio_map (PCIDevice *d,int region_num,uint32_t addr,uint32_t size,int type) { -CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga; +CirrusVGAState *s =DO_UPCAST (CirrusVGAState, dev, d);cpu_register_physical_memory (addr,CIRRUS_PNPMMIO_SIZE , s->cirrus_mmio_io_addr);
@@ -3293,26 +3289,24 @@ static void cirrus_pci_mmio_map(PCIDevice *d, int region_num,static void pci_cirrus_write_config (PCIDevice *d,uint32_t address,uint32_t val,int len) { -PCICirrusVGAState *pvs =container_of (d, PCICirrusVGAState, dev); -CirrusVGAState *s = &pvs->cirrus_vga; +CirrusVGAState *s =DO_UPCAST (CirrusVGAState, dev, d);pci_default_write_config (d, address, val, len); -if (s->vga.map_addr && pvs->dev.io_regions[0 ].addr ==-1 ) +if (s->vga.map_addr && s->dev.io_regions[0 ].addr ==-1 ) s->vga.map_addr =0 ;cirrus_update_memory_access (s); }static void pci_cirrus_vga_initfn (PCIDevice *dev) { -PCICirrusVGAState *d =DO_UPCAST (PCICirrusVGAState, dev, dev); -CirrusVGAState *s = &d->cirrus_vga; -uint8_t *pci_conf = d->dev.config; +CirrusVGAState *s =DO_UPCAST (CirrusVGAState, dev, dev); +uint8_t *pci_conf = s->dev.config;int device_id =CIRRUS_ID_CLGD5446 ;/* setup VGA */ vga_common_init (&s->vga,VGA_RAM_SIZE );cirrus_init_common (s, device_id,1 ); - s->vga.pci_dev = (PCIDevice *)d; + s->vga.pci_dev = dev; s->vga.ds =graphic_console_init (s->vga.update, s->vga.invalidate, s->vga.screen_dump, s->vga.text_update, &s->vga);
@@ -3328,10 +3322,10 @@ static void pci_cirrus_vga_initfn(PCIDevice *dev)/* memory #0 LFB */ /* memory #1 memory-mapped I/O */ /* XXX: s->vga.vram_size must be a power of two */ -pci_register_bar ((PCIDevice *)d,0 ,0x2000000 , +pci_register_bar (&s->dev,0 ,0x2000000 ,PCI_ADDRESS_SPACE_MEM_PREFETCH , cirrus_pci_lfb_map);if (device_id ==CIRRUS_ID_CLGD5446 ) { -pci_register_bar ((PCIDevice *)d,1 ,CIRRUS_PNPMMIO_SIZE , +pci_register_bar (&s->dev,1 ,CIRRUS_PNPMMIO_SIZE ,PCI_ADDRESS_SPACE_MEM , cirrus_pci_mmio_map); }/* XXX: ROM BIOS */
@@ -3344,7 +3338,7 @@ void pci_cirrus_vga_init(PCIBus *bus)static PCIDeviceInfo cirrus_vga_info = { .qdev.name ="Cirrus VGA" , - .qdev.size =sizeof (PCICirrusVGAState), + .qdev.size =sizeof (CirrusVGAState), .init = pci_cirrus_vga_initfn, .config_write = pci_cirrus_write_config, };
Signature · 1 line
1.6.2.5
PATCH 18/22 cirrus_vga: remove pointless cast from void *
In reply to Juan Quintela--- hw/cirrus_vga.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
hw/cirrus_vga.c +5 -5
@@ -2342,7 +2342,7 @@ static void cirrus_cursor_draw_line(VGAState *s1, uint8_t *d1, int scr_y)static uint32_t cirrus_linear_readb (void *opaque,target_phys_addr_t addr) { -CirrusVGAState *s = (CirrusVGAState *) opaque; +CirrusVGAState *s = opaque;uint32_t ret; addr &= s->cirrus_addr_mask;
@@ -2401,7 +2401,7 @@ static uint32_t cirrus_linear_readl(void *opaque, target_phys_addr_t addr)static void cirrus_linear_writeb (void *opaque,target_phys_addr_t addr,uint32_t val) { -CirrusVGAState *s = (CirrusVGAState *) opaque; +CirrusVGAState *s = opaque;unsigned mode; addr &= s->cirrus_addr_mask;
@@ -2529,7 +2529,7 @@ static uint32_t cirrus_linear_bitblt_readl(void *opaque, target_phys_addr_t addrstatic void cirrus_linear_bitblt_writeb (void *opaque,target_phys_addr_t addr,uint32_t val) { -CirrusVGAState *s = (CirrusVGAState *) opaque; +CirrusVGAState *s = opaque;if (s->cirrus_srcptr != s->cirrus_srcptr_end) {/* bitblt */
@@ -2915,7 +2915,7 @@ static void vga_ioport_write(void *opaque, uint32_t addr, uint32_t val)static uint32_t cirrus_mmio_readb (void *opaque,target_phys_addr_t addr) { -CirrusVGAState *s = (CirrusVGAState *) opaque; +CirrusVGAState *s = opaque; addr &=CIRRUS_PNPMMIO_SIZE -1 ;
@@ -2959,7 +2959,7 @@ static uint32_t cirrus_mmio_readl(void *opaque, target_phys_addr_t addr)static void cirrus_mmio_writeb (void *opaque,target_phys_addr_t addr,uint32_t val) { -CirrusVGAState *s = (CirrusVGAState *) opaque; +CirrusVGAState *s = opaque; addr &=CIRRUS_PNPMMIO_SIZE -1 ;
Signature · 1 line
1.6.2.5
PATCH 19/22 cirrus_vga: change use of pci_dev for is_pci
In reply to Juan Quintela--- hw/cirrus_vga.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
hw/cirrus_vga.c +6 -5
@@ -241,6 +241,7 @@ typedef struct CirrusVGAState {PCIDevice dev;VGACommonState vga; +int is_pci;int cirrus_linear_io_addr;int cirrus_linear_bitblt_io_addr;int cirrus_mmio_io_addr;
@@ -3017,8 +3018,8 @@ static void cirrus_vga_save(QEMUFile *f, void *opaque) {CirrusVGAState *s = opaque; -if (s->vga.pci_dev) -pci_device_save (s->vga.pci_dev, f); +if (s->is_pci) +pci_device_save (&s->dev, f);qemu_put_be32s (f, &s->vga.latch);qemu_put_8s (f, &s->vga.sr_index);
@@ -3063,8 +3064,8 @@ static int cirrus_vga_load(QEMUFile *f, void *opaque, int version_id)if (version_id >2 )return -EINVAL ; -if (s->vga.pci_dev && version_id >=2 ) { - ret =pci_device_load (s->vga.pci_dev, f); +if (s->is_pci && version_id >=2 ) { + ret =pci_device_load (&s->dev, f);if (ret <0 )return ret; }
@@ -3306,7 +3307,7 @@ static void pci_cirrus_vga_initfn(PCIDevice *dev)/* setup VGA */ vga_common_init (&s->vga,VGA_RAM_SIZE );cirrus_init_common (s, device_id,1 ); - s->vga.pci_dev = dev; + s->is_pci =1 ; s->vga.ds =graphic_console_init (s->vga.update, s->vga.invalidate, s->vga.screen_dump, s->vga.text_update, &s->vga);
Signature · 1 line
1.6.2.5
PATCH 20/22 Introduce vga_common_reset() to be able to typcheck vga_reset()
In reply to Juan Quintela--- hw/cirrus_vga.c | 2 +- hw/vga.c | 10 +++++++--- hw/vga_int.h | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-)
hw/cirrus_vga.c +1 -1
@@ -3122,7 +3122,7 @@ static void cirrus_reset(void *opaque) {CirrusVGAState *s = opaque; -vga_reset (&s->vga); +vga_common_reset (&s->vga);unmap_linear_vram (s); s->vga.sr[0x06 ] =0x0f ;if (s->device_id ==CIRRUS_ID_CLGD5446 ) {
hw/vga.c +7 -3
@@ -1871,10 +1871,8 @@ static void vga_invalidate_display(void *opaque) s->full_update =1 ; } -void vga_reset (void *opaque) +void vga_common_reset (VGACommonState *s) { -VGAState *s = (VGAState *) opaque; - s->lfb_addr =0 ; s->lfb_end =0 ; s->map_addr =0 ;
@@ -1940,6 +1938,12 @@ void vga_reset(void *opaque) } } +static void vga_reset (void *opaque) +{ +VGAState *s = (VGAState *) opaque; +vga_common_reset (s); +} +#define TEXTMODE_X (x) ((x) % width)#define TEXTMODE_Y (x) ((x) / width)#define VMEM2CHTYPE (v) ((v & 0xff0007ff) | \
hw/vga_int.h +1 -1
@@ -192,7 +192,7 @@ static inline int c6_to_8(int v)void vga_common_init (VGAState *s,int vga_ram_size);void vga_init (VGAState *s); -void vga_reset (void *s); +void vga_common_reset (VGACommonState *s);void vga_dirty_log_start (VGAState *s);
Signature · 1 line
1.6.2.5
PATCH 21/22 vga: Rename vga_state -> vga
In reply to Juan Quintela--- hw/vga.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
hw/vga.c +6 -6
@@ -2225,7 +2225,7 @@ static int vga_load(QEMUFile *f, void *opaque, int version_id)typedef struct PCIVGAState {PCIDevice dev; -VGAState vga_state; +VGAState vga; }PCIVGAState ;void vga_dirty_log_start (VGAState *s)
@@ -2243,7 +2243,7 @@ static void vga_map(PCIDevice *pci_dev, int region_num, uint32_t addr, uint32_t size, int type) { PCIVGAState *d = (PCIVGAState *)pci_dev; -VGAState *s = &d->vga_state; +VGAState *s = &d->vga;if (region_num ==PCI_ROM_SLOT ) {cpu_register_physical_memory (addr, s->bios_size, s->bios_offset); }else {
@@ -2477,7 +2477,7 @@ static void pci_vga_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) { PCIVGAState *pvs = container_of(d, PCIVGAState, dev); -VGAState *s = &pvs->vga_state; +VGAState *s = &pvs->vga;pci_default_write_config (d, address, val, len);if (s->map_addr && pvs->dev.io_regions[0 ].addr ==-1 )
@@ -2487,7 +2487,7 @@ static void pci_vga_write_config(PCIDevice *d,static void pci_vga_initfn (PCIDevice *dev) {PCIVGAState *d =DO_UPCAST (PCIVGAState, dev, dev); -VGAState *s = &d->vga_state; +VGAState *s = &d->vga;uint8_t *pci_conf = d->dev.config;// vga + console init
@@ -2537,8 +2537,8 @@ static PCIDeviceInfo vga_info = { .init = pci_vga_initfn, .config_write = pci_vga_write_config, .qdev.props = (Property []) { -DEFINE_PROP_HEX32 ("bios-offset" , PCIVGAState, vga_state.bios_offset,0 ), -DEFINE_PROP_HEX32 ("bios-size" , PCIVGAState, vga_state.bios_size,0 ), +DEFINE_PROP_HEX32 ("bios-offset" , PCIVGAState, vga.bios_offset,0 ), +DEFINE_PROP_HEX32 ("bios-size" , PCIVGAState, vga.bios_size,0 ),DEFINE_PROP_END_OF_LIST (), } };
Signature · 1 line
1.6.2.5
PATCH 22/22 Everything outside of vga.c should use VGACommonState
In reply to Juan Quintela--- hw/cirrus_vga.c | 10 +++++----- hw/vga.c | 4 +++- hw/vga_int.h | 10 ++++------ hw/vmware_vga.c | 4 ++-- 4 files changed, 14 insertions(+), 14 deletions(-)
hw/cirrus_vga.c +5 -5
@@ -1075,7 +1075,7 @@ static void cirrus_write_bitblt(CirrusVGAState * s, unsigned reg_value)* ***************************************/ -static void cirrus_get_offsets (VGAState *s1, +static void cirrus_get_offsets (VGACommonState *s1,uint32_t *pline_offset,uint32_t *pstart_addr,uint32_t *pline_compare)
@@ -1123,7 +1123,7 @@ static uint32_t cirrus_get_bpp16_depth(CirrusVGAState * s)return ret; } -static int cirrus_get_bpp (VGAState *s1) +static int cirrus_get_bpp (VGACommonState *s1) {CirrusVGAState * s =container_of (s1, CirrusVGAState, vga);uint32_t ret =8 ;
@@ -1161,7 +1161,7 @@ static int cirrus_get_bpp(VGAState *s1)return ret; } -static void cirrus_get_resolution (VGAState *s,int *pwidth,int *pheight) +static void cirrus_get_resolution (VGACommonState *s,int *pwidth,int *pheight) {int width, height;
@@ -2231,7 +2231,7 @@ static inline void cirrus_cursor_compute_yrange(CirrusVGAState *s)/* NOTE: we do not currently handle the cursor bitmap change, so we update the cursor only if it moves. */ -static void cirrus_cursor_invalidate (VGAState *s1) +static void cirrus_cursor_invalidate (VGACommonState *s1) {CirrusVGAState *s =container_of (s1, CirrusVGAState, vga);int size;
@@ -2260,7 +2260,7 @@ static void cirrus_cursor_invalidate(VGAState *s1) } } -static void cirrus_cursor_draw_line (VGAState *s1,uint8_t *d1,int scr_y) +static void cirrus_cursor_draw_line (VGACommonState *s1,uint8_t *d1,int scr_y) {CirrusVGAState *s =container_of (s1, CirrusVGAState, vga);int w, h, bpp, x1, x2, poffset;
hw/vga.c +3 -1
@@ -149,6 +149,8 @@ static uint32_t expand4[256];static uint16_t expand2[256 ];static uint8_t expand4to8[16 ]; +typedef VGACommonState VGAState ; +static void vga_screen_dump (void *opaque,const char *filename);static void vga_dumb_update_retrace_info (VGAState *s)
@@ -2254,7 +2256,7 @@ static void vga_map(PCIDevice *pci_dev, int region_num, } } -void vga_common_init (VGAState *s,int vga_ram_size) +void vga_common_init (VGACommonState *s,int vga_ram_size) {int i, j, v, b;
hw/vga_int.h +4 -6
@@ -180,8 +180,6 @@ typedef struct VGACommonState {union vga_retrace retrace_info; } VGACommonState; -typedef VGACommonState VGAState ; -static inline int c6_to_8 (int v) {int b;
@@ -190,15 +188,15 @@ static inline int c6_to_8(int v)return (v <<2 ) | (b <<1 ) | b; } -void vga_common_init (VGAState *s,int vga_ram_size); -void vga_init (VGAState *s); +void vga_common_init (VGACommonState *s,int vga_ram_size); +void vga_init (VGACommonState *s);void vga_common_reset (VGACommonState *s); -void vga_dirty_log_start (VGAState *s); +void vga_dirty_log_start (VGACommonState *s);uint32_t vga_mem_readb (void *opaque,target_phys_addr_t addr);void vga_mem_writeb (void *opaque,target_phys_addr_t addr,uint32_t val); -void vga_invalidate_scanlines (VGAState *s,int y1,int y2); +void vga_invalidate_scanlines (VGACommonState *s,int y1,int y2);int ppm_save (const char *filename,struct DisplaySurface *ds); void vga_draw_cursor_line_8(uint8_t *d1, const uint8_t *src1,
hw/vmware_vga.c +2 -2
@@ -1129,8 +1129,8 @@ static void vmsvga_init(struct vmsvga_state_s *s, int vga_ram_size)vmsvga_reset (s);#ifdef EMBED_STDVGA -vga_common_init ((VGAState *) s, vga_ram_size); -vga_init ((VGAState *) s); +vga_common_init (&s->vga, vga_ram_size); +vga_init (&s->vga);#else s->vram_size = vga_ram_size; s->vram_offset =qemu_ram_alloc (vga_ram_size);
Signature · 1 line
1.6.2.5
Quoted 4 lines · Juan Quintela
This patch series clean up "half" converted qemu drivers that had changed from:
struct FOOState
Quoted 8 lines
to
typedef PCIFOOState { PCIDevice dev; FOOState foo; } PCIFOOState;It just moves PCIDevice to be the 1st field of FOOState.
Note that there are a bunch of cases where it actually makes sense to have PCIFOOState because foo can be connected to different busses and thus there is a ISAFOOState (or will be once ISA is qdevified too).
Disclaimer: did't look through the patches yet, just scanned the subjects. But I've seen ne2000 + vga on the list. For both devices ISA variants exist. Likewise OHCI, there are also non-PCI variants.
Quoted 2 lines
- pcnet: It needs a different approach, because it can be both a PCIDevice or a SysBus device.
That one too ;)
Why do you want to change it?
cheers,
Gerd
Quoted 14 lines · Gerd Hoffmann
Quoted 4 lines · Juan Quintela
This patch series clean up "half" converted qemu drivers that had changed from:
struct FOOState
Quoted 8 lines
to
typedef PCIFOOState { PCIDevice dev; FOOState foo; } PCIFOOState;It just moves PCIDevice to be the 1st field of FOOState.
Quoted 7 lines
Note that there are a bunch of cases where it actually makes sense to have PCIFOOState because foo can be connected to different busses and thus there is a ISAFOOState (or will be once ISA is qdevified too).
Disclaimer: did't look through the patches yet, just scanned the subjects. But I've seen ne2000 + vga on the list. For both devices ISA variants exist. Likewise OHCI, there are also non-PCI variants.
on the PCI/ISA difference:
* ne2000: just looked again at it, we could probably found a way to get that around.
* vga: this needs major refactoring.
* OHCI: I did the change, and everything compiled, didn't found the other uses :p
I didn't changed it because then I had to replicate the save/load functions. (looking at it again, it appears that this ones are the only diff)
Quoted 5 lines
Quoted 2 lines
- pcnet: It needs a different approach, because it can be both a PCIDevice or a SysBus device.
That one too ;)
Why do you want to change it?
PCNetState first field is
PCIDevice *pci_dev.
and that device is also for lance. Will take a look at your approach for diferentaiting them.
Later, Juan.