[Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup

[Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup

[Greg Kurz]

---

Greg Kurz (2):
      spapr_pci: convert sprintf() to g_strdup_printf()
      spapr_pci: don't create 64-bit MMIO window if we don't need to


 hw/ppc/spapr_pci.c |   37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/2] spapr_pci: convert sprintf() to g_strdup_printf()

[Greg Kurz]

In order to follow a QEMU common practice.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 75cd9392233e..7d84b9766ed2 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1609,34 +1609,37 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
 
     sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
 
-    namebuf = alloca(strlen(sphb->dtbusname) + 32);
-
     /* Initialize memory regions */
-    sprintf(namebuf, "%s.mmio", sphb->dtbusname);
+    namebuf = g_strdup_printf("%s.mmio", sphb->dtbusname);
     memory_region_init(&sphb->memspace, OBJECT(sphb), namebuf, UINT64_MAX);
+    g_free(namebuf);
 
-    sprintf(namebuf, "%s.mmio32-alias", sphb->dtbusname);
+    namebuf = g_strdup_printf("%s.mmio32-alias", sphb->dtbusname);
     memory_region_init_alias(&sphb->mem32window, OBJECT(sphb),
                              namebuf, &sphb->memspace,
                              SPAPR_PCI_MEM_WIN_BUS_OFFSET, sphb->mem_win_size);
+    g_free(namebuf);
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->mem32window);
 
-    sprintf(namebuf, "%s.mmio64-alias", sphb->dtbusname);
+    namebuf = g_strdup_printf("%s.mmio64-alias", sphb->dtbusname);
     memory_region_init_alias(&sphb->mem64window, OBJECT(sphb),
                              namebuf, &sphb->memspace,
                              sphb->mem64_win_pciaddr, sphb->mem64_win_size);
+    g_free(namebuf);
     memory_region_add_subregion(get_system_memory(), sphb->mem64_win_addr,
                                 &sphb->mem64window);
 
     /* Initialize IO regions */
-    sprintf(namebuf, "%s.io", sphb->dtbusname);
+    namebuf = g_strdup_printf("%s.io", sphb->dtbusname);
     memory_region_init(&sphb->iospace, OBJECT(sphb),
                        namebuf, SPAPR_PCI_IO_WIN_SIZE);
+    g_free(namebuf);
 
-    sprintf(namebuf, "%s.io-alias", sphb->dtbusname);
+    namebuf = g_strdup_printf("%s.io-alias", sphb->dtbusname);
     memory_region_init_alias(&sphb->iowindow, OBJECT(sphb), namebuf,
                              &sphb->iospace, 0, SPAPR_PCI_IO_WIN_SIZE);
+    g_free(namebuf);
     memory_region_add_subregion(get_system_memory(), sphb->io_win_addr,
                                 &sphb->iowindow);
 
@@ -1654,10 +1657,10 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
      * Later the guest might want to create another DMA window
      * which will become another memory subregion.
      */
-    sprintf(namebuf, "%s.iommu-root", sphb->dtbusname);
-
+    namebuf = g_strdup_printf("%s.iommu-root", sphb->dtbusname);
     memory_region_init(&sphb->iommu_root, OBJECT(sphb),
                        namebuf, UINT64_MAX);
+    g_free(namebuf);
     address_space_init(&sphb->iommu_as, &sphb->iommu_root,
                        sphb->dtbusname);
 

[Qemu-devel] [PATCH 2/2] spapr_pci: don't create 64-bit MMIO window if we don't need to

[Greg Kurz]

When running a pseries-2.2 or older machine type, we get the following
lines in info mtree:

address-space: memory
...
ffffffffffffffff-ffffffffffffffff (prio 0, i/o): alias
 pci@800000020000000.mmio64-alias @pci@800000020000000.mmio
  ffffffffffffffff-ffffffffffffffff

address-space: cpu-memory
...
ffffffffffffffff-ffffffffffffffff (prio 0, i/o): alias
 pci@800000020000000.mmio64-alias @pci@800000020000000.mmio
  ffffffffffffffff-ffffffffffffffff

The same thing occurs when running a pseries-2.7 with

    -global spapr-pci-host-bridge.mem_win_size=2147483648

This happens because we always create a 64-bit MMIO window, even if
we didn't explicitely requested it (ie, mem64_win_size == 0) and the
32-bit window is below 2GiB. It doesn't seem to have an impact on the
guest though because spapr_populate_pci_dt() doesn't advertise the
bogus windows when mem64_win_size == 0.

Since these memory regions don't induce any state, we can safely
choose to not create them when their address is equal to -1,
without breaking migration from existing setups.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_pci.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7d84b9766ed2..cf54160526fa 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1622,13 +1622,19 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     memory_region_add_subregion(get_system_memory(), sphb->mem_win_addr,
                                 &sphb->mem32window);
 
-    namebuf = g_strdup_printf("%s.mmio64-alias", sphb->dtbusname);
-    memory_region_init_alias(&sphb->mem64window, OBJECT(sphb),
-                             namebuf, &sphb->memspace,
-                             sphb->mem64_win_pciaddr, sphb->mem64_win_size);
-    g_free(namebuf);
-    memory_region_add_subregion(get_system_memory(), sphb->mem64_win_addr,
-                                &sphb->mem64window);
+    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
+        namebuf = g_strdup_printf("%s.mmio64-alias", sphb->dtbusname);
+        memory_region_init_alias(&sphb->mem64window, OBJECT(sphb),
+                                 namebuf, &sphb->memspace,
+                                 sphb->mem64_win_pciaddr, sphb->mem64_win_size);
+        g_free(namebuf);
+
+        if (sphb->mem64_win_addr != (hwaddr)-1) {
+            memory_region_add_subregion(get_system_memory(),
+                                        sphb->mem64_win_addr,
+                                        &sphb->mem64window);
+        }
+    }
 
     /* Initialize IO regions */
     namebuf = g_strdup_printf("%s.io", sphb->dtbusname);

Re: [Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup
Message-id: 150512483616.8642.15831240906912354238.stgit@bahia.lan
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/150512483616.8642.15831240906912354238.stgit@bahia.lan -> patchew/150512483616.8642.15831240906912354238.stgit@bahia.lan
Switched to a new branch 'test'
da1ec6583a spapr_pci: don't create 64-bit MMIO window if we don't need to
f9e9150977 spapr_pci: convert sprintf() to g_strdup_printf()

=== OUTPUT BEGIN ===
Checking PATCH 1/2: spapr_pci: convert sprintf() to g_strdup_printf()...
Checking PATCH 2/2: spapr_pci: don't create 64-bit MMIO window if we don't need to...
ERROR: spaces required around that '-' (ctx:VxV)
#54: FILE: hw/ppc/spapr_pci.c:1650:
+    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
                                            ^

ERROR: spaces required around that '-' (ctx:VxV)
#61: FILE: hw/ppc/spapr_pci.c:1657:
+        if (sphb->mem64_win_addr != (hwaddr)-1) {
                                             ^

total: 2 errors, 0 warnings, 26 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup

[Greg Kurz]

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

On Mon, 11 Sep 2017 03:17:45 -0700 (PDT)
no-reply@patchew.org wrote:

> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup
> Message-id: 150512483616.8642.15831240906912354238.stgit@bahia.lan
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]               patchew/150512483616.8642.15831240906912354238.stgit@bahia.lan -> patchew/150512483616.8642.15831240906912354238.stgit@bahia.lan
> Switched to a new branch 'test'
> da1ec6583a spapr_pci: don't create 64-bit MMIO window if we don't need to
> f9e9150977 spapr_pci: convert sprintf() to g_strdup_printf()
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/2: spapr_pci: convert sprintf() to g_strdup_printf()...
> Checking PATCH 2/2: spapr_pci: don't create 64-bit MMIO window if we don't need to...
> ERROR: spaces required around that '-' (ctx:VxV)
> #54: FILE: hw/ppc/spapr_pci.c:1650:
> +    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
>                                             ^
> 
> ERROR: spaces required around that '-' (ctx:VxV)
> #61: FILE: hw/ppc/spapr_pci.c:1657:
> +        if (sphb->mem64_win_addr != (hwaddr)-1) {
> 

False positive... Fixable with:

--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -213,6 +213,7 @@ our @typeList = (
        qr{${Ident}_handler},
        qr{${Ident}_handler_fn},
        qr{target_(?:u)?long},
+       qr{hwaddr},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful

                                             ^


> 
> total: 2 errors, 0 warnings, 26 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

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

Re: [Qemu-devel] [PATCH 0/2] spapr_pci: some more cleanup

[David Gibson]

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

On Mon, Sep 11, 2017 at 12:13:56PM +0200, Greg Kurz wrote:
> ---
> 
> Greg Kurz (2):
>       spapr_pci: convert sprintf() to g_strdup_printf()
>       spapr_pci: don't create 64-bit MMIO window if we don't need to

Applied to ppc-for-2.11.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]