[Qemu-devel] [PATCH 00/11] various easy cleanups

[Qemu-devel] [PATCH 00/11] various easy cleanups

[Philippe Mathieu-Daudé]

various easy patchs added while coding around:
- remove old comments
- add/remove includes
- use TYPE_ names when available
- use ARRAY_SIZE() macro

Philippe Mathieu-Daudé (11):
  hw/net: removed obsolete comments
  hw/pci: define msi_nonbroken in pci-stub
  hw/misc: add missing includes
  ide/ahci: add missing includes
  hw/mips: add missing include
  hw/arm: removed unnecessary include
  hw/arm: use defined type name instead of hard-coded string.
  hw/sparc: use ARRAY_SIZE() macro
  target/sparc: fix DEBUG_MMU DPRINTF() arguments
  register: display register prefix (name) since it is available
  MAINTAINERS: self-appoint me as reviewer of the Register API

 MAINTAINERS                |  1 +
 hw/arm/armv7m.c            |  4 ++--
 hw/arm/exynos4210.c        |  4 ++--
 hw/arm/highbank.c          | 11 +++++++----
 hw/arm/realview.c          |  6 ++++--
 hw/arm/vexpress.c          |  6 ++++--
 hw/arm/xilinx_zynq.c       | 14 ++++++++------
 hw/core/register.c         |  8 ++++----
 hw/net/pcnet-pci.c         | 10 ----------
 hw/pci/pci-stub.c          |  3 +++
 hw/sparc64/sun4u.c         |  2 +-
 include/hw/arm/arm.h       |  1 -
 include/hw/ide/ahci.h      |  2 ++
 include/hw/mips/mips.h     |  1 +
 include/hw/misc/unimp.h    |  3 +++
 target/sparc/ldst_helper.c |  4 ++--
 16 files changed, 44 insertions(+), 36 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 01/11] hw/net: removed obsolete comments

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/pcnet-pci.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 0acf8a4879..bdb533436e 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -38,14 +38,6 @@
 
 #include "pcnet.h"
 
-//#define PCNET_DEBUG
-//#define PCNET_DEBUG_IO
-//#define PCNET_DEBUG_BCR
-//#define PCNET_DEBUG_CSR
-//#define PCNET_DEBUG_RMD
-//#define PCNET_DEBUG_TMD
-//#define PCNET_DEBUG_MATCH
-
 #define TYPE_PCI_PCNET "pcnet"
 
 #define PCI_PCNET(obj) \
@@ -239,8 +231,6 @@ static const VMStateDescription vmstate_pci_pcnet = {
     }
 };
 
-/* PCI interface */
-
 static const MemoryRegionOps pcnet_mmio_ops = {
     .old_mmio = {
         .read = { pcnet_mmio_readb, pcnet_mmio_readw, pcnet_mmio_readl },
-- 
2.11.0

Re: [Qemu-devel] [PATCH 01/11] hw/net: removed obsolete comments

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/pcnet-pci.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
> index 0acf8a4879..bdb533436e 100644
> --- a/hw/net/pcnet-pci.c
> +++ b/hw/net/pcnet-pci.c
> @@ -38,14 +38,6 @@
>
>  #include "pcnet.h"
>
> -//#define PCNET_DEBUG
> -//#define PCNET_DEBUG_IO
> -//#define PCNET_DEBUG_BCR
> -//#define PCNET_DEBUG_CSR
> -//#define PCNET_DEBUG_RMD
> -//#define PCNET_DEBUG_TMD
> -//#define PCNET_DEBUG_MATCH
> -

I guess - although I notice there are some #if 0's in the file that
could be converted to proper non-bitrotting debug prints or removed as well.

>  #define TYPE_PCI_PCNET "pcnet"
>
>  #define PCI_PCNET(obj) \
> @@ -239,8 +231,6 @@ static const VMStateDescription vmstate_pci_pcnet = {
>      }
>  };
>
> -/* PCI interface */
> -

Was this intentional?

>  static const MemoryRegionOps pcnet_mmio_ops = {
>      .old_mmio = {
>          .read = { pcnet_mmio_readb, pcnet_mmio_readw, pcnet_mmio_readl },


--
Alex Bennée

[Qemu-devel] [RFC PATCH 02/11] hw/pci: define msi_nonbroken in pci-stub

[Philippe Mathieu-Daudé]

This field is accessed in hw/intc/arm_gicv[23*].c

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/pci/pci-stub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index 36d2c430c5..ecad664946 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -24,6 +24,9 @@
 #include "qapi/qmp/qerror.h"
 #include "hw/pci/pci.h"
 #include "qmp-commands.h"
+#include "hw/pci/msi.h"
+
+bool msi_nonbroken;
 
 PciInfoList *qmp_query_pci(Error **errp)
 {
-- 
2.11.0

Re: [Qemu-devel] [RFC PATCH 02/11] hw/pci: define msi_nonbroken in pci-stub

[Thomas Huth]

On 09.05.2017 01:39, Philippe Mathieu-Daudé wrote:
> This field is accessed in hw/intc/arm_gicv[23*].c

default-configs/arm-softmmu.mak sets CONFIG_PCI, so this should not be
necessary, I think. Otherwise, you should extend your patch description,
to elaborate on what you're trying to do here.

 Thomas

Re: [Qemu-devel] [RFC PATCH 02/11] hw/pci: define msi_nonbroken in pci-stub

[Philippe Mathieu-Daudé]

Hi Thomas,

On 05/09/2017 02:19 AM, Thomas Huth wrote:
> On 09.05.2017 01:39, Philippe Mathieu-Daudé wrote:
>> This field is accessed in hw/intc/arm_gicv[23*].c
>
> default-configs/arm-softmmu.mak sets CONFIG_PCI, so this should not be
> necessary, I think. Otherwise, you should extend your patch description,
> to elaborate on what you're trying to do here.
>
>  Thomas

Sure, I'm willing to add basic support for the TI Hercules MCU which is 
a big endian SoC (without PCI support).

I'm using the following config:

$ cat default-configs/armeb-softmmu.mak
# Default configuration for armeb-softmmu
CONFIG_ECC=y
CONFIG_SERIAL=y
CONFIG_PTIMER=y
CONFIG_SD=y
CONFIG_SSI=y
#CONFIG_USB=y
CONFIG_PLATFORM_BUS=y
CONFIG_ARM_V7M=y
CONFIG_ARM_GIC=y
CONFIG_ARM_TIMER=y
CONFIG_PL011=y
CONFIG_PL022=y
CONFIG_PL031=y
CONFIG_PL041=y
CONFIG_PL050=y
CONFIG_PL061=y
CONFIG_PL080=y
CONFIG_PL181=y
CONFIG_PL190=y
CONFIG_PL310=y
CONFIG_PL330=y
CONFIG_BITBANG_I2C=y
CONFIG_GPIO_KEY=y
CONFIG_HERCULES=y

I had to modify default-configs/arm-softmmu.mak to restrict a bit 
hw/arm/Makefile.objs, see:

https://github.com/qemu/qemu/compare/master...philmd:armeb_wip

In this branch I run:

$ ../configure --target-list=armeb-softmmu && make -j
[...]
   LINK    armeb-softmmu/qemu-system-armeb
../hw/intc/arm_gicv2m.o: In function `gicv2m_realize':
qemu/hw/intc/arm_gicv2m.c:154: undefined reference to `msi_nonbroken'
../hw/intc/arm_gicv3_its_common.o: In function `gicv3_its_init_mmio':
qemu/hw/intc/arm_gicv3_its_common.c:110: undefined reference to 
`msi_nonbroken'
collect2: error: ld returned 1 exit status
Makefile:201: recipe for target 'qemu-system-armeb' failed
make[1]: *** [qemu-system-armeb] Error 1
Makefile:327: recipe for target 'subdir-armeb-softmmu' failed
make: *** [subdir-armeb-softmmu] Error 2

This MCU does not use a GIC so I can also remove it, but this error 
seems odd to me.

As I understand the 'msi_nonbroken' is a kludge (there is no intrinsic 
relationship between PCI/MSI and this particular interrupt controller).
Since it is declared as extern in "hw/pci/msi.h" why not define it in 
pci-stub?

Regards,

Phil.

Re: [Qemu-devel] [RFC PATCH 02/11] hw/pci: define msi_nonbroken in pci-stub

[Thomas Huth]

 Hi Philippe,

On 09.05.2017 18:49, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 05/09/2017 02:19 AM, Thomas Huth wrote:
>> On 09.05.2017 01:39, Philippe Mathieu-Daudé wrote:
>>> This field is accessed in hw/intc/arm_gicv[23*].c
>>
>> default-configs/arm-softmmu.mak sets CONFIG_PCI, so this should not be
>> necessary, I think. Otherwise, you should extend your patch description,
>> to elaborate on what you're trying to do here.
>>
>>  Thomas
> 
> Sure, I'm willing to add basic support for the TI Hercules MCU which is
> a big endian SoC (without PCI support).
> 
> I'm using the following config:
> 
> $ cat default-configs/armeb-softmmu.mak

$ cat default-configs/armeb-softmmu.mak
cat: default-configs/armeb-softmmu.mak: No such file or directory

... so you're planning for a new armeb config file here? That's the kind
of information that I was missing in the patch description.

[...]
> As I understand the 'msi_nonbroken' is a kludge (there is no intrinsic
> relationship between PCI/MSI and this particular interrupt controller).
> Since it is declared as extern in "hw/pci/msi.h" why not define it in
> pci-stub?

Sure, your patch is fine, IMHO it just lacks some rationale in its
description.

 Thomas

[Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/unimp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
index 3462d85836..353ee19abf 100644
--- a/include/hw/misc/unimp.h
+++ b/include/hw/misc/unimp.h
@@ -8,6 +8,9 @@
 #ifndef HW_MISC_UNIMP_H
 #define HW_MISC_UNIMP_H
 
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+
 #define TYPE_UNIMPLEMENTED_DEVICE "unimplemented-device"
 
 /**
-- 
2.11.0

Re: [Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Eric Blake]

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

On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/misc/unimp.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
> index 3462d85836..353ee19abf 100644
> --- a/include/hw/misc/unimp.h
> +++ b/include/hw/misc/unimp.h
> @@ -8,6 +8,9 @@
>  #ifndef HW_MISC_UNIMP_H
>  #define HW_MISC_UNIMP_H
>  
> +#include "qemu/osdep.h"

NACK. .h files should not include osdep.h, because the .c file that is
using the .h file should have already done so.  This is mentioned in
HACKING.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Philippe Mathieu-Daudé]

On 05/08/2017 08:56 PM, Eric Blake wrote:
> On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/misc/unimp.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
>> index 3462d85836..353ee19abf 100644
>> --- a/include/hw/misc/unimp.h
>> +++ b/include/hw/misc/unimp.h
>> @@ -8,6 +8,9 @@
>>  #ifndef HW_MISC_UNIMP_H
>>  #define HW_MISC_UNIMP_H
>>
>> +#include "qemu/osdep.h"
>
> NACK. .h files should not include osdep.h, because the .c file that is
> using the .h file should have already done so.  This is mentioned in
> HACKING.

Ok! Indeed my .c doesn't include osdep.h, sorry for the noise.

Re: [Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Philippe Mathieu-Daudé]

Hi Eric,

On 05/08/2017 09:56 PM, Philippe Mathieu-Daudé wrote:
> On 05/08/2017 08:56 PM, Eric Blake wrote:
>> On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  include/hw/misc/unimp.h | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
>>> index 3462d85836..353ee19abf 100644
>>> --- a/include/hw/misc/unimp.h
>>> +++ b/include/hw/misc/unimp.h
>>> @@ -8,6 +8,9 @@
>>>  #ifndef HW_MISC_UNIMP_H
>>>  #define HW_MISC_UNIMP_H
>>>
>>> +#include "qemu/osdep.h"
>>
>> NACK. .h files should not include osdep.h, because the .c file that is
>> using the .h file should have already done so.  This is mentioned in
>> HACKING.
>
> Ok! Indeed my .c doesn't include osdep.h, sorry for the noise.

I attentively read the HACKING after a good night's sleep.

What about the other include, "hw/sysbus.h"?
The inlined function create_unimplemented_device() calls 
sysbus_mmio_map_overlap(). Anyone willing to use "hw/misc/unimp.h" will 
get a compilation failure if he does not include "hw/sysbus.h" and I 
don't think it belongs to "qemu/osdep.h".

Regards,

Phil.

Re: [Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Eric Blake]

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

On 05/09/2017 12:20 PM, Philippe Mathieu-Daudé wrote:

> What about the other include, "hw/sysbus.h"?
> The inlined function create_unimplemented_device() calls
> sysbus_mmio_map_overlap(). Anyone willing to use "hw/misc/unimp.h" will
> get a compilation failure if he does not include "hw/sysbus.h" and I
> don't think it belongs to "qemu/osdep.h".

That one's probably okay.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Philippe Mathieu-Daudé]

On 05/08/2017 08:56 PM, Eric Blake wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/misc/unimp.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/include/hw/misc/unimp.h b/include/hw/misc/unimp.h
>> index 3462d85836..353ee19abf 100644
>> --- a/include/hw/misc/unimp.h
>> +++ b/include/hw/misc/unimp.h
>> @@ -8,6 +8,9 @@
>>   #ifndef HW_MISC_UNIMP_H
>>   #define HW_MISC_UNIMP_H
>>   
>> +#include "qemu/osdep.h"
> 
> NACK. .h files should not include osdep.h, because the .c file that is
> using the .h file should have already done so.  This is mentioned in
> HACKING.

I'm not sure this kind of mistake happens often enough, but it might 
worth add a such test in checkpatch.pl (easier to say than to implement :p)

Re: [Qemu-devel] [PATCH 03/11] hw/misc: add missing includes

[Peter Maydell]

On 5 September 2017 at 10:35, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I'm not sure this kind of mistake happens often enough, but it might worth
> add a such test in checkpatch.pl (easier to say than to implement :p)

It doesn't run automatically, but you can run scripts/clean-includes
on your new files to remove unneeded includes and add osdep.h if it's
missing.

thanks
-- PMM

[Qemu-devel] [PATCH 04/11] ide/ahci: add missing includes

[Philippe Mathieu-Daudé]

qemu/include/hw/ide/ahci.h:260:16: error: field ‘sglist’ has incomplete type
     QEMUSGList sglist;
                ^~~~~~
/qemu/include/hw/ide/ahci.h:272:5: error: unknown type name ‘IDEDMA’
     IDEDMA dma;
     ^~~~~~
qemu/include/hw/ide/ahci.h:273:5: error: unknown type name ‘IDEBus’
     IDEBus port;
     ^~~~~~
qemu/include/hw/ide/ahci.h:305:15: error: field ‘parent_obj’ has incomplete type
     PCIDevice parent_obj;
               ^~~~~~~~~~

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/ide/ahci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 0ca7c65820..293f9ebcd0 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -25,6 +25,8 @@
 #define HW_IDE_AHCI_H
 
 #include "hw/sysbus.h"
+#include "hw/ide/internal.h"
+#include "sysemu/dma.h"
 
 #define AHCI_MEM_BAR_SIZE         0x1000
 #define AHCI_MAX_PORTS            32
-- 
2.11.0

Re: [Qemu-devel] [PATCH 04/11] ide/ahci: add missing includes

[Eric Blake]

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

On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
> qemu/include/hw/ide/ahci.h:260:16: error: field ‘sglist’ has incomplete type
>      QEMUSGList sglist;
>                 ^~~~~~

What are you doing to get this compilation error (configure options,
platform, compiler, etc)?  I can't reproduce it. Is it something that
pops up later when you remove includes from somewhere else, and you're
just pre-emptively adding includes here to allow removal of includes later?

> /qemu/include/hw/ide/ahci.h:272:5: error: unknown type name ‘IDEDMA’
>      IDEDMA dma;
>      ^~~~~~
> qemu/include/hw/ide/ahci.h:273:5: error: unknown type name ‘IDEBus’
>      IDEBus port;
>      ^~~~~~
> qemu/include/hw/ide/ahci.h:305:15: error: field ‘parent_obj’ has incomplete type
>      PCIDevice parent_obj;
>                ^~~~~~~~~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/ide/ahci.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
> index 0ca7c65820..293f9ebcd0 100644
> --- a/include/hw/ide/ahci.h
> +++ b/include/hw/ide/ahci.h
> @@ -25,6 +25,8 @@
>  #define HW_IDE_AHCI_H
>  
>  #include "hw/sysbus.h"
> +#include "hw/ide/internal.h"
> +#include "sysemu/dma.h"
>  
>  #define AHCI_MEM_BAR_SIZE         0x1000
>  #define AHCI_MAX_PORTS            32
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 04/11] ide/ahci: add missing includes

[John Snow]

On 05/08/2017 07:58 PM, Eric Blake wrote:
> On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
>> qemu/include/hw/ide/ahci.h:260:16: error: field ‘sglist’ has incomplete type
>>      QEMUSGList sglist;
>>                 ^~~~~~
> 
> What are you doing to get this compilation error (configure options,
> platform, compiler, etc)?  I can't reproduce it. Is it something that
> pops up later when you remove includes from somewhere else, and you're
> just pre-emptively adding includes here to allow removal of includes later?
> 

"What he said," but if I had to guess, Philippe is testing that each
header can be compiled separately.

>> /qemu/include/hw/ide/ahci.h:272:5: error: unknown type name ‘IDEDMA’
>>      IDEDMA dma;
>>      ^~~~~~
>> qemu/include/hw/ide/ahci.h:273:5: error: unknown type name ‘IDEBus’
>>      IDEBus port;
>>      ^~~~~~
>> qemu/include/hw/ide/ahci.h:305:15: error: field ‘parent_obj’ has incomplete type
>>      PCIDevice parent_obj;
>>                ^~~~~~~~~~
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/ide/ahci.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
>> index 0ca7c65820..293f9ebcd0 100644
>> --- a/include/hw/ide/ahci.h
>> +++ b/include/hw/ide/ahci.h
>> @@ -25,6 +25,8 @@
>>  #define HW_IDE_AHCI_H
>>  
>>  #include "hw/sysbus.h"
>> +#include "hw/ide/internal.h">> +#include "sysemu/dma.h"
>>  
>>  #define AHCI_MEM_BAR_SIZE         0x1000
>>  #define AHCI_MAX_PORTS            32
>>
> 

[Qemu-devel] [RFC PATCH 0/2] ide/ahci: add missing includes

[Philippe Mathieu-Daudé]

Hi Eric, John.

The patch 07/11 "hw/arm: use defined type name instead of hard-coded string."
triggered this problem.

I split the previous series to only include the 2 commits involved to reproduce.

As you will see, calxeda_init() from highbank.c uses the sysbus type name
"sysbus-ahci" which is already defined in "hw/ide/ahci.h" as TYPE_SYSBUS_AHCI.
As mentioned in HACKING highbank.c first include is "qemu/osdep.h". However
compiling highbank.c fails (as reported in patch "04/11" commit comments).

I don't think appropriate to include "hw/ide/internal.h" and "sysemu/dma.h" in
highbank.c. Eventually another source including "hw/ide/ahci.h" will hit the
same error.

Do you see another place where those includes should go?

I think I did a mistake sending these both patchs in a series thinking they were
unrelated, I should have send them altogether, my bad :/

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  arm/highbank: use defined type name instead of hard-coded string
  ide/ahci: add missing includes

 hw/arm/highbank.c     | 3 ++-
 include/hw/ide/ahci.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

-- 
2.11.0

[Qemu-devel] [RFC PATCH 1/2] arm/highbank: use defined type name instead of hard-coded string

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/highbank.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index d209b97dee..dd809b404b 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -31,6 +31,7 @@
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
 #include "hw/char/pl011.h"
+#include "hw/ide/ahci.h"
 
 #define SMP_BOOT_ADDR           0x100
 #define SMP_BOOT_REG            0x40
@@ -341,7 +342,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     sysbus_create_simple("pl031", 0xfff35000, pic[19]);
     sysbus_create_simple("pl022", 0xfff39000, pic[23]);
 
-    sysbus_create_simple("sysbus-ahci", 0xffe08000, pic[83]);
+    sysbus_create_simple(TYPE_SYSBUS_AHCI, 0xffe08000, pic[83]);
 
     if (nd_table[0].used) {
         qemu_check_nic_model(&nd_table[0], "xgmac");
-- 
2.11.0

Re: [Qemu-devel] [RFC PATCH 1/2] arm/highbank: use defined type name instead of hard-coded string

[John Snow]

On 06/09/2017 11:05 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/highbank.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index d209b97dee..dd809b404b 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -31,6 +31,7 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/error-report.h"
>  #include "hw/char/pl011.h"
> +#include "hw/ide/ahci.h"
>  
>  #define SMP_BOOT_ADDR           0x100
>  #define SMP_BOOT_REG            0x40
> @@ -341,7 +342,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>      sysbus_create_simple("pl031", 0xfff35000, pic[19]);
>      sysbus_create_simple("pl022", 0xfff39000, pic[23]);
>  
> -    sysbus_create_simple("sysbus-ahci", 0xffe08000, pic[83]);
> +    sysbus_create_simple(TYPE_SYSBUS_AHCI, 0xffe08000, pic[83]);
>  
>      if (nd_table[0].used) {
>          qemu_check_nic_model(&nd_table[0], "xgmac");
> 

This patch is fine, I took a stab at fixing the include issues in a
separate series. Try it all out and let me know.

--js

Re: [Qemu-devel] [RFC PATCH 1/2] arm/highbank: use defined type name instead of hard-coded string

[Philippe Mathieu-Daudé]

On Fri, Jun 23, 2017 at 7:18 PM, John Snow <jsnow@redhat.com> wrote:
> This patch is fine, I took a stab at fixing the include issues in a
> separate series. Try it all out and let me know.

Thank John, your series does fix my include issues :)

[Qemu-devel] [RFC PATCH 2/2] ide/ahci: add missing includes

[Philippe Mathieu-Daudé]

  CC      arm-softmmu/hw/arm/highbank.o
In file included from qemu/hw/arm/highbank.c:34:0:
qemu/include/hw/ide/ahci.h:260:16: error: field ‘sglist’ has incomplete type
     QEMUSGList sglist;
                ^~~~~~
/qemu/include/hw/ide/ahci.h:272:5: error: unknown type name ‘IDEDMA’
     IDEDMA dma;
     ^~~~~~
qemu/include/hw/ide/ahci.h:273:5: error: unknown type name ‘IDEBus’
     IDEBus port;
     ^~~~~~
qemu/include/hw/ide/ahci.h:305:15: error: field ‘parent_obj’ has incomplete type
     PCIDevice parent_obj;
               ^~~~~~~~~~
qemu/rules.mak:69: recipe for target 'hw/arm/highbank.o' failed
make[1]: *** [hw/arm/highbank.o] Error 1
make: *** [subdir-arm-softmmu] Error 2

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/ide/ahci.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/ide/ahci.h b/include/hw/ide/ahci.h
index 0ca7c65820..293f9ebcd0 100644
--- a/include/hw/ide/ahci.h
+++ b/include/hw/ide/ahci.h
@@ -25,6 +25,8 @@
 #define HW_IDE_AHCI_H
 
 #include "hw/sysbus.h"
+#include "hw/ide/internal.h"
+#include "sysemu/dma.h"
 
 #define AHCI_MEM_BAR_SIZE         0x1000
 #define AHCI_MAX_PORTS            32
-- 
2.11.0

[Qemu-devel] [PATCH 05/11] hw/mips: add missing include

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/mips/mips.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
index e0065ce808..16412dc150 100644
--- a/include/hw/mips/mips.h
+++ b/include/hw/mips/mips.h
@@ -6,6 +6,7 @@
 #define INITRD_PAGE_MASK (~((1 << 16) - 1))
 
 #include "exec/memory.h"
+#include "hw/irq.h"
 
 /* gt64xxx.c */
 PCIBus *gt64120_register(qemu_irq *pic);
-- 
2.11.0

Re: [Qemu-devel] [PATCH 05/11] hw/mips: add missing include

[Aurelien Jarno]

On 2017-05-08 20:39, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/mips/mips.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/hw/mips/mips.h b/include/hw/mips/mips.h
> index e0065ce808..16412dc150 100644
> --- a/include/hw/mips/mips.h
> +++ b/include/hw/mips/mips.h
> @@ -6,6 +6,7 @@
>  #define INITRD_PAGE_MASK (~((1 << 16) - 1))
>  
>  #include "exec/memory.h"
> +#include "hw/irq.h"
>  
>  /* gt64xxx.c */
>  PCIBus *gt64120_register(qemu_irq *pic);

Acked-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [Qemu-trivial] [PATCH 05/11] hw/mips: add missing include

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

[Qemu-devel] [PATCH 06/11] hw/arm: removed unnecessary include

[Philippe Mathieu-Daudé]

"exec/memory.h" already includes it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/arm.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index a3f79d3379..b9c11d3fb8 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -14,7 +14,6 @@
 #include "exec/memory.h"
 #include "target/arm/cpu-qom.h"
 #include "hw/irq.h"
-#include "qemu/notify.h"
 
 typedef enum {
     ARM_ENDIANNESS_UNKNOWN = 0,
-- 
2.11.0

Re: [Qemu-devel] [PATCH 06/11] hw/arm: removed unnecessary include

[Michael Tokarev]

09.05.2017 02:39, Philippe Mathieu-Daudé wrote:
> "exec/memory.h" already includes it.

I think this sort of include cleanups isn't really necessary.
For a regular include file, it's better to include it if the
.c file in question uses definitions from there, no matter if
other .h files include that header already or not.

Thanks,

/mjt

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/arm.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index a3f79d3379..b9c11d3fb8 100644
> --- a/include/hw/arm/arm.h
> +++ b/include/hw/arm/arm.h
> @@ -14,7 +14,6 @@
>  #include "exec/memory.h"
>  #include "target/arm/cpu-qom.h"
>  #include "hw/irq.h"
> -#include "qemu/notify.h"
>  
>  typedef enum {
>      ARM_ENDIANNESS_UNKNOWN = 0,
> 

Re: [Qemu-devel] [PATCH 06/11] hw/arm: removed unnecessary include

[Philippe Mathieu-Daudé]

On 05/23/2017 11:58 AM, Michael Tokarev wrote:
> 09.05.2017 02:39, Philippe Mathieu-Daudé wrote:
>> "exec/memory.h" already includes it.
>
> I think this sort of include cleanups isn't really necessary.

Some are useful ;)

> For a regular include file, it's better to include it if the
> .c file in question uses definitions from there, no matter if
> other .h files include that header already or not.

Ok. This one was incorrect anyway :( thank for the review!

>
> Thanks,
>
> /mjt
>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/arm/arm.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
>> index a3f79d3379..b9c11d3fb8 100644
>> --- a/include/hw/arm/arm.h
>> +++ b/include/hw/arm/arm.h
>> @@ -14,7 +14,6 @@
>>  #include "exec/memory.h"
>>  #include "target/arm/cpu-qom.h"
>>  #include "hw/irq.h"
>> -#include "qemu/notify.h"
>>
>>  typedef enum {
>>      ARM_ENDIANNESS_UNKNOWN = 0,
>>
>

[Qemu-devel] [PATCH 08/11] hw/sparc: use ARRAY_SIZE() macro

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sparc64/sun4u.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index d347b6616d..525d6f44a0 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -214,7 +214,7 @@ static void isa_irq_handler(void *opaque, int n, int level)
     qemu_irq *irqs = opaque;
     int ivec;
 
-    assert(n < 16);
+    assert(n < ARRAY_SIZE(isa_irq_to_ivec));
     ivec = isa_irq_to_ivec[n];
     EBUS_DPRINTF("Set ISA IRQ %d level %d -> ivec 0x%x\n", n, level, ivec);
     if (ivec) {
-- 
2.11.0

Re: [Qemu-devel] [PATCH 08/11] hw/sparc: use ARRAY_SIZE() macro

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sparc64/sun4u.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index d347b6616d..525d6f44a0 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -214,7 +214,7 @@ static void isa_irq_handler(void *opaque, int n, int level)
>      qemu_irq *irqs = opaque;
>      int ivec;
>
> -    assert(n < 16);
> +    assert(n < ARRAY_SIZE(isa_irq_to_ivec));
>      ivec = isa_irq_to_ivec[n];
>      EBUS_DPRINTF("Set ISA IRQ %d level %d -> ivec 0x%x\n", n, level, ivec);
>      if (ivec) {

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

--
Alex Bennée

Re: [Qemu-devel] [PATCH 08/11] hw/sparc: use ARRAY_SIZE() macro

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

[Qemu-devel] [PATCH 09/11] target/sparc: fix DEBUG_MMU DPRINTF() arguments

[Philippe Mathieu-Daudé]

invalid since 96df2bc99f9

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/sparc/ldst_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
index 57968d9143..aa83a49a88 100644
--- a/target/sparc/ldst_helper.c
+++ b/target/sparc/ldst_helper.c
@@ -1631,7 +1631,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
 
             if (oldreg != env->immu.mmuregs[reg]) {
                 DPRINTF_MMU("immu change reg[%d]: 0x%016" PRIx64 " -> 0x%016"
-                            PRIx64 "\n", reg, oldreg, env->immuregs[reg]);
+                            PRIx64 "\n", reg, oldreg, env->immu.mmuregs[reg]);
             }
 #ifdef DEBUG_MMU
             dump_mmu(stdout, fprintf, env);
@@ -1715,7 +1715,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
 
             if (oldreg != env->dmmu.mmuregs[reg]) {
                 DPRINTF_MMU("dmmu change reg[%d]: 0x%016" PRIx64 " -> 0x%016"
-                            PRIx64 "\n", reg, oldreg, env->dmmuregs[reg]);
+                            PRIx64 "\n", reg, oldreg, env->dmmu.mmuregs[reg]);
             }
 #ifdef DEBUG_MMU
             dump_mmu(stdout, fprintf, env);
-- 
2.11.0

Re: [Qemu-devel] [PATCH 09/11] target/sparc: fix DEBUG_MMU DPRINTF() arguments

[Eric Blake]

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

On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
> invalid since 96df2bc99f9
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/sparc/ldst_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
> index 57968d9143..aa83a49a88 100644
> --- a/target/sparc/ldst_helper.c
> +++ b/target/sparc/ldst_helper.c
> @@ -1631,7 +1631,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
>  
>              if (oldreg != env->immu.mmuregs[reg]) {
>                  DPRINTF_MMU("immu change reg[%d]: 0x%016" PRIx64 " -> 0x%016"
> -                            PRIx64 "\n", reg, oldreg, env->immuregs[reg]);
> +                            PRIx64 "\n", reg, oldreg, env->immu.mmuregs[reg]);

Please take this opportunity to fix the broken definitions of
DPRINTF_MMU() and friends so that they don't bit-rot when not defined.
There are plenty of other examples of re-writing broken #defines to
instead favor an if (0) { printf(...) } to benefit from -Wformat
checking even when the debugging is disabled.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 09/11] target/sparc: fix DEBUG_MMU DPRINTF() arguments

[Philippe Mathieu-Daudé]

On 05/08/2017 09:00 PM, Eric Blake wrote:
> On 05/08/2017 06:39 PM, Philippe Mathieu-Daudé wrote:
>> invalid since 96df2bc99f9
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  target/sparc/ldst_helper.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/sparc/ldst_helper.c b/target/sparc/ldst_helper.c
>> index 57968d9143..aa83a49a88 100644
>> --- a/target/sparc/ldst_helper.c
>> +++ b/target/sparc/ldst_helper.c
>> @@ -1631,7 +1631,7 @@ void helper_st_asi(CPUSPARCState *env, target_ulong addr, target_ulong val,
>>
>>              if (oldreg != env->immu.mmuregs[reg]) {
>>                  DPRINTF_MMU("immu change reg[%d]: 0x%016" PRIx64 " -> 0x%016"
>> -                            PRIx64 "\n", reg, oldreg, env->immuregs[reg]);
>> +                            PRIx64 "\n", reg, oldreg, env->immu.mmuregs[reg]);
>
> Please take this opportunity to fix the broken definitions of
> DPRINTF_MMU() and friends so that they don't bit-rot when not defined.
> There are plenty of other examples of re-writing broken #defines to
> instead favor an if (0) { printf(...) } to benefit from -Wformat
> checking even when the debugging is disabled.

Sure, will do it.

[Qemu-devel] [PATCH 10/11] register: display register prefix (name) since it is available

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/register.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/core/register.c b/hw/core/register.c
index dc335a79a9..b5988c9cc3 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -195,8 +195,8 @@ void register_write_memory(void *opaque, hwaddr addr,
     }
 
     if (!reg) {
-        qemu_log_mask(LOG_GUEST_ERROR, "Write to unimplemented register at " \
-                      "address: %#" PRIx64 "\n", addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to unimplemented register " \
+                      "at address: %#" PRIx64 "\n", reg_array->prefix, addr);
         return;
     }
 
@@ -224,8 +224,8 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
     }
 
     if (!reg) {
-        qemu_log_mask(LOG_GUEST_ERROR, "Read to unimplemented register at " \
-                      "address: %#" PRIx64 "\n", addr);
+        qemu_log_mask(LOG_GUEST_ERROR, "%s:  read to unimplemented register " \
+                      "at address: %#" PRIx64 "\n", reg_array->prefix, addr);
         return 0;
     }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH 10/11] register: display register prefix (name) since it is available

[Alistair Francis]

On Mon, May 8, 2017 at 4:39 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
>  hw/core/register.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index dc335a79a9..b5988c9cc3 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -195,8 +195,8 @@ void register_write_memory(void *opaque, hwaddr addr,
>      }
>
>      if (!reg) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "Write to unimplemented register at " \
> -                      "address: %#" PRIx64 "\n", addr);
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: write to unimplemented register " \
> +                      "at address: %#" PRIx64 "\n", reg_array->prefix, addr);
>          return;
>      }
>
> @@ -224,8 +224,8 @@ uint64_t register_read_memory(void *opaque, hwaddr addr,
>      }
>
>      if (!reg) {
> -        qemu_log_mask(LOG_GUEST_ERROR, "Read to unimplemented register at " \
> -                      "address: %#" PRIx64 "\n", addr);
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s:  read to unimplemented register " \
> +                      "at address: %#" PRIx64 "\n", reg_array->prefix, addr);
>          return 0;
>      }
>
> --
> 2.11.0
>
>

Re: [Qemu-devel] [PATCH 10/11] register: display register prefix (name) since it is available

[Michael Tokarev]

applied to -trivial, thanks!

/mjt

[Qemu-devel] [PATCH 11/11] MAINTAINERS: self-appoint me as reviewer of the Register API

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index efdec47319..de32165059 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1431,6 +1431,7 @@ T: git git://repo.or.cz/qemu/armbru.git qapi-next
 
 Register API
 M: Alistair Francis <alistair.francis@xilinx.com>
+R: Philippe Mathieu-Daudé <f4bug@amsat.org>
 S: Maintained
 F: hw/core/register.c
 F: include/hw/register.h
-- 
2.11.0

Re: [Qemu-devel] [PATCH 00/11] various easy cleanups

[Michael Tokarev]

09.05.2017 02:39, Philippe Mathieu-Daudé wrote:
> various easy patchs added while coding around:
> - remove old comments
> - add/remove includes
> - use TYPE_ names when available
> - use ARRAY_SIZE() macro
> 
> Philippe Mathieu-Daudé (11):
>   hw/net: removed obsolete comments
>   hw/pci: define msi_nonbroken in pci-stub
>   hw/misc: add missing includes
>   ide/ahci: add missing includes
>   hw/mips: add missing include
>   hw/arm: removed unnecessary include
>   hw/arm: use defined type name instead of hard-coded string.
>   hw/sparc: use ARRAY_SIZE() macro
>   target/sparc: fix DEBUG_MMU DPRINTF() arguments
>   register: display register prefix (name) since it is available
>   MAINTAINERS: self-appoint me as reviewer of the Register API

>From this set, I've applied 4 patches:
 [PATCH 05/11] hw/mips: add missing include
 [PATCH 06/11] hw/arm: removed unnecessary include
 [PATCH 08/11] hw/sparc: use ARRAY_SIZE() macro
 [PATCH 10/11] register: display register prefix (name) since it is available

Thanks,

/mjt