[v1] sun4m: sparc32_dma tidy-ups

[Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups

Posted by Mark Cave-Ayland 6 years, 6 months ago

This patchset aims to tidy-up the sparc32_dma code by improving the
modelling of the espdma/ledma devices using both QOM and the memory
API which didn't exist when the code was first written.

The result is that it is now possible to remove both the iommu_opaque
and is_ledma workarounds from the code, and the code for wiring up
the espdma/ledma and respective devices is also a lot more readable.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v4:
- Rebase onto git master
- Update patch 9 to move lance QOM macros/SysBusPCNetState from sun4m.h to lance.h as suggested by Peter
- Add Reviewed-by from Peter for patch 7

v3:
- Add missing sysbus.h include to esp.h in patch 7

v2:
- Make esp/lance devices children of espdma/ledma devices respectively
- Add len parameter to ledma/espdma tracepoints


Mark Cave-Ayland (13):
  sparc32_dma: rename SPARC32_DMA type to SPARC32_DMA_DEVICE
  sparc32_dma: split esp and le into separate DMA devices
  sparc32_dma: move type declarations from sparc32_dma.c to
    sparc32_dma.h
  sun4m: move DMA device wiring from sparc32_dma_init() to
    sun4m_hw_init()
  sun4m_iommu: move TYPE_SUN4M_IOMMU declaration to sun4m.h
  sparc32_dma: use object link instead of qdev property to pass IOMMU
    reference
  esp: move TYPE_ESP and SysBusESPState from esp.c to esp.h
  sparc32_dma: make esp device child of espdma device
  lance: move TYPE_LANCE and SysBusPCNetState from lance.c to lance.h
  sparc32_dma: make lance device child of ledma device
  sparc32_dma: introduce new SPARC32_DMA type container object
  sparc32_dma: remove is_ledma hack and replace with memory region
    alias
  sparc32_dma: add len to esp/le DMA memory tracing

 hw/dma/sparc32_dma.c           |  235 +++++++++++++++++++++++++++++-----------
 hw/dma/sun4m_iommu.c           |   14 ---
 hw/dma/trace-events            |    8 +-
 hw/net/lance.c                 |   11 +-
 hw/scsi/esp.c                  |   13 ---
 hw/sparc/sun4m.c               |   82 ++++++--------
 include/hw/net/lance.h         |   41 +++++++
 include/hw/scsi/esp.h          |   14 +++
 include/hw/sparc/sparc32_dma.h |   55 ++++++++++
 include/hw/sparc/sun4m.h       |   16 +++
 10 files changed, 336 insertions(+), 153 deletions(-)
 create mode 100644 include/hw/net/lance.h

-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups

Posted by Artyom Tarasenko 6 years, 6 months ago

On Wed, Oct 25, 2017 at 5:59 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> This patchset aims to tidy-up the sparc32_dma code by improving the
> modelling of the espdma/ledma devices using both QOM and the memory
> API which didn't exist when the code was first written.
>
> The result is that it is now possible to remove both the iommu_opaque
> and is_ledma workarounds from the code, and the code for wiring up
> the espdma/ledma and respective devices is also a lot more readable.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>

>
> v4:
> - Rebase onto git master
> - Update patch 9 to move lance QOM macros/SysBusPCNetState from sun4m.h to lance.h as suggested by Peter
> - Add Reviewed-by from Peter for patch 7
>
> v3:
> - Add missing sysbus.h include to esp.h in patch 7
>
> v2:
> - Make esp/lance devices children of espdma/ledma devices respectively
> - Add len parameter to ledma/espdma tracepoints
>
>
> Mark Cave-Ayland (13):
>   sparc32_dma: rename SPARC32_DMA type to SPARC32_DMA_DEVICE
>   sparc32_dma: split esp and le into separate DMA devices
>   sparc32_dma: move type declarations from sparc32_dma.c to
>     sparc32_dma.h
>   sun4m: move DMA device wiring from sparc32_dma_init() to
>     sun4m_hw_init()
>   sun4m_iommu: move TYPE_SUN4M_IOMMU declaration to sun4m.h
>   sparc32_dma: use object link instead of qdev property to pass IOMMU
>     reference
>   esp: move TYPE_ESP and SysBusESPState from esp.c to esp.h
>   sparc32_dma: make esp device child of espdma device
>   lance: move TYPE_LANCE and SysBusPCNetState from lance.c to lance.h
>   sparc32_dma: make lance device child of ledma device
>   sparc32_dma: introduce new SPARC32_DMA type container object
>   sparc32_dma: remove is_ledma hack and replace with memory region
>     alias
>   sparc32_dma: add len to esp/le DMA memory tracing
>
>  hw/dma/sparc32_dma.c           |  235 +++++++++++++++++++++++++++++-----------
>  hw/dma/sun4m_iommu.c           |   14 ---
>  hw/dma/trace-events            |    8 +-
>  hw/net/lance.c                 |   11 +-
>  hw/scsi/esp.c                  |   13 ---
>  hw/sparc/sun4m.c               |   82 ++++++--------
>  include/hw/net/lance.h         |   41 +++++++
>  include/hw/scsi/esp.h          |   14 +++
>  include/hw/sparc/sparc32_dma.h |   55 ++++++++++
>  include/hw/sparc/sun4m.h       |   16 +++
>  10 files changed, 336 insertions(+), 153 deletions(-)
>  create mode 100644 include/hw/net/lance.h
>
> --
> 1.7.10.4
>



-- 
Regards,
Artyom Tarasenko

SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu

Re: [Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups

Posted by Philippe Mathieu-Daudé 6 years, 6 months ago

Hi Mark,

On 10/25/2017 12:59 PM, Mark Cave-Ayland wrote:
> This patchset aims to tidy-up the sparc32_dma code by improving the
> modelling of the espdma/ledma devices using both QOM and the memory
> API which didn't exist when the code was first written.
> 
> The result is that it is now possible to remove both the iommu_opaque
> and is_ledma workarounds from the code, and the code for wiring up
> the espdma/ledma and respective devices is also a lot more readable.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

The whole series:

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

If you don't accept my comments (or don't have time) about keeping
"hw/sparc/sparc32_dma.h" generic and moving network/scsi parts in
"hw/sparc/sun4m.h" you can still add to your series:

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

Also while testing your series on a Debian image, I noted your series
results faster, I timed:

master: 104s
your series: 85s (>20% faster!)

Regards,

Phil.

> 
> v4:
> - Rebase onto git master
> - Update patch 9 to move lance QOM macros/SysBusPCNetState from sun4m.h to lance.h as suggested by Peter
> - Add Reviewed-by from Peter for patch 7
> 
> v3:
> - Add missing sysbus.h include to esp.h in patch 7
> 
> v2:
> - Make esp/lance devices children of espdma/ledma devices respectively
> - Add len parameter to ledma/espdma tracepoints
> 
> 
> Mark Cave-Ayland (13):
>   sparc32_dma: rename SPARC32_DMA type to SPARC32_DMA_DEVICE
>   sparc32_dma: split esp and le into separate DMA devices
>   sparc32_dma: move type declarations from sparc32_dma.c to
>     sparc32_dma.h
>   sun4m: move DMA device wiring from sparc32_dma_init() to
>     sun4m_hw_init()
>   sun4m_iommu: move TYPE_SUN4M_IOMMU declaration to sun4m.h
>   sparc32_dma: use object link instead of qdev property to pass IOMMU
>     reference
>   esp: move TYPE_ESP and SysBusESPState from esp.c to esp.h
>   sparc32_dma: make esp device child of espdma device
>   lance: move TYPE_LANCE and SysBusPCNetState from lance.c to lance.h
>   sparc32_dma: make lance device child of ledma device
>   sparc32_dma: introduce new SPARC32_DMA type container object
>   sparc32_dma: remove is_ledma hack and replace with memory region
>     alias
>   sparc32_dma: add len to esp/le DMA memory tracing
> 
>  hw/dma/sparc32_dma.c           |  235 +++++++++++++++++++++++++++++-----------
>  hw/dma/sun4m_iommu.c           |   14 ---
>  hw/dma/trace-events            |    8 +-
>  hw/net/lance.c                 |   11 +-
>  hw/scsi/esp.c                  |   13 ---
>  hw/sparc/sun4m.c               |   82 ++++++--------
>  include/hw/net/lance.h         |   41 +++++++
>  include/hw/scsi/esp.h          |   14 +++
>  include/hw/sparc/sparc32_dma.h |   55 ++++++++++
>  include/hw/sparc/sun4m.h       |   16 +++
>  10 files changed, 336 insertions(+), 153 deletions(-)
>  create mode 100644 include/hw/net/lance.h
>

Re: [Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups

Posted by Mark Cave-Ayland 6 years, 5 months ago

On 27/10/17 17:42, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 10/25/2017 12:59 PM, Mark Cave-Ayland wrote:
>> This patchset aims to tidy-up the sparc32_dma code by improving the
>> modelling of the espdma/ledma devices using both QOM and the memory
>> API which didn't exist when the code was first written.
>>
>> The result is that it is now possible to remove both the iommu_opaque
>> and is_ledma workarounds from the code, and the code for wiring up
>> the espdma/ledma and respective devices is also a lot more readable.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> The whole series:
> 
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> If you don't accept my comments (or don't have time) about keeping
> "hw/sparc/sparc32_dma.h" generic and moving network/scsi parts in
> "hw/sparc/sun4m.h" you can still add to your series:
> 
> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks for the review, I've added your R-B tags to the individual
patches. Note that while potentially I could move the network/scsi parts
to hw/sparc/sun4m.h I feel that it's a slightly better match for the
SPARC32 DMA container device to remain in sparc32_dma.c. So for these
patches I've just added your A-B tag.

> Also while testing your series on a Debian image, I noted your series
> results faster, I timed:
> 
> master: 104s
> your series: 85s (>20% faster!)

Really? Is that for just this patchset or also with the v2 IOMMU
patchset applied on top? I can't immediately see how moving the logic
into sparc32_dma.c could make a difference here...

ATB,

Mark.

Re: [Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups

Posted by Philippe Mathieu-Daudé 6 years, 5 months ago

On Mon, Oct 30, 2017 at 4:00 PM, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 27/10/17 17:42, Philippe Mathieu-Daudé wrote:
[...]
>> If you don't accept my comments (or don't have time) about keeping
>> "hw/sparc/sparc32_dma.h" generic and moving network/scsi parts in
>> "hw/sparc/sun4m.h" you can still add to your series:
>>
>> Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Thanks for the review, I've added your R-B tags to the individual
> patches. Note that while potentially I could move the network/scsi parts
> to hw/sparc/sun4m.h I feel that it's a slightly better match for the
> SPARC32 DMA container device to remain in sparc32_dma.c. So for these
> patches I've just added your A-B tag.

OK.

>> Also while testing your series on a Debian image, I noted your series
>> results faster, I timed:
>>
>> master: 104s
>> your series: 85s (>20% faster!)
>
> Really? Is that for just this patchset or also with the v2 IOMMU
> patchset applied on top? I can't immediately see how moving the logic
> into sparc32_dma.c could make a difference here...

Yes, I was trying with both series applied, so this comment belong to
the other series (IOMMU).

Regards,

Phil.

Re: [Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups

Posted by no-reply@patchew.org 6 years, 6 months ago

Hi,

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

Subject: [Qemu-devel] [PATCHv4 00/13] sun4m: sparc32_dma tidy-ups
Type: series
Message-id: 1508947167-5304-1-git-send-email-mark.cave-ayland@ilande.co.uk

=== 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/1508947167-5304-1-git-send-email-mark.cave-ayland@ilande.co.uk -> patchew/1508947167-5304-1-git-send-email-mark.cave-ayland@ilande.co.uk
Switched to a new branch 'test'
36eda9dd73 sparc32_dma: add len to esp/le DMA memory tracing
ac63079d23 sparc32_dma: remove is_ledma hack and replace with memory region alias
0e813fbc8a sparc32_dma: introduce new SPARC32_DMA type container object
2ac77fa8c0 sparc32_dma: make lance device child of ledma device
67c364cb3a lance: move TYPE_LANCE and SysBusPCNetState from lance.c to lance.h
63a69fb7ff sparc32_dma: make esp device child of espdma device
6aab399e1b esp: move TYPE_ESP and SysBusESPState from esp.c to esp.h
378f5fe6a5 sparc32_dma: use object link instead of qdev property to pass IOMMU reference
a927040955 sun4m_iommu: move TYPE_SUN4M_IOMMU declaration to sun4m.h
8367f3cdd0 sun4m: move DMA device wiring from sparc32_dma_init() to sun4m_hw_init()
1b4868953f sparc32_dma: move type declarations from sparc32_dma.c to sparc32_dma.h
018f7bd135 sparc32_dma: split esp and le into separate DMA devices
2ca43fd787 sparc32_dma: rename SPARC32_DMA type to SPARC32_DMA_DEVICE

=== OUTPUT BEGIN ===
Checking PATCH 1/13: sparc32_dma: rename SPARC32_DMA type to SPARC32_DMA_DEVICE...
Checking PATCH 2/13: sparc32_dma: split esp and le into separate DMA devices...
Checking PATCH 3/13: sparc32_dma: move type declarations from sparc32_dma.c to sparc32_dma.h...
Checking PATCH 4/13: sun4m: move DMA device wiring from sparc32_dma_init() to sun4m_hw_init()...
ERROR: spaces required around that '*' (ctx:WxV)
#52: FILE: hw/sparc/sun4m.c:824:
+    qemu_irq *cpu_irqs[MAX_CPUS], slavio_irq[32], slavio_cpu_irq[MAX_CPUS];
              ^

total: 1 errors, 0 warnings, 66 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.

Checking PATCH 5/13: sun4m_iommu: move TYPE_SUN4M_IOMMU declaration to sun4m.h...
Checking PATCH 6/13: sparc32_dma: use object link instead of qdev property to pass IOMMU reference...
Checking PATCH 7/13: esp: move TYPE_ESP and SysBusESPState from esp.c to esp.h...
Checking PATCH 8/13: sparc32_dma: make esp device child of espdma device...
Checking PATCH 9/13: lance: move TYPE_LANCE and SysBusPCNetState from lance.c to lance.h...
Checking PATCH 10/13: sparc32_dma: make lance device child of ledma device...
Checking PATCH 11/13: sparc32_dma: introduce new SPARC32_DMA type container object...
Checking PATCH 12/13: sparc32_dma: remove is_ledma hack and replace with memory region alias...
Checking PATCH 13/13: sparc32_dma: add len to esp/le DMA memory tracing...
=== 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