[Qemu-devel] [PATCH 0/2] scsi: remove legacy esp_init() function

[Qemu-devel] [PATCH 0/2] scsi: remove legacy esp_init() function

[Mark Cave-Ayland]

Something else that came out of reviewing Laurent's q800 patchset: after my
SPARC cleanups last year, MIPS Jazz is the last remaining user of the legacy
esp_init() function.

Patch 1 switches mips_jazz_init() over to create the ESP device directly
via qdev. Please note that I do not have any MIPS jazz images and so this
is compile-tested only (although passes "make check") and needs an ACK from
someone (Hervé?).

Now that the last user of esp_init() is gone, patch 2 removes the legacy
function and its associated header.

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


Mark Cave-Ayland (2):
  hw/mips/jazz: create ESP device directly via qdev
  esp: remove legacy esp_init() function

 hw/mips/mips_jazz.c   | 19 ++++++++++++++++---
 hw/scsi/esp.c         | 30 ------------------------------
 include/hw/scsi/esp.h |  5 -----
 3 files changed, 16 insertions(+), 38 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Mark Cave-Ayland]

MIPS jazz is the last user of the legacy esp_init() function so move creation
of the ESP device over to use qdev.

Note that the esp_reset and dma_enable qemu_irqs are currently unused and so
we do not wire these up and instead remove the variables to prevent the
compiler emitting unused variable warnings.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/mips/mips_jazz.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/mips/mips_jazz.c b/hw/mips/mips_jazz.c
index 90cb306f53..1afbe3ce6a 100644
--- a/hw/mips/mips_jazz.c
+++ b/hw/mips/mips_jazz.c
@@ -145,10 +145,10 @@ static void mips_jazz_init(MachineState *machine,
     ISABus *isa_bus;
     ISADevice *pit;
     DriveInfo *fds[MAX_FD];
-    qemu_irq esp_reset, dma_enable;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     MemoryRegion *bios = g_new(MemoryRegion, 1);
     MemoryRegion *bios2 = g_new(MemoryRegion, 1);
+    SysBusESPState *sysbus_esp;
     ESPState *esp;
 
     /* init CPUs */
@@ -281,8 +281,21 @@ static void mips_jazz_init(MachineState *machine,
     }
 
     /* SCSI adapter */
-    esp = esp_init(0x80002000, 0, rc4030_dma_read, rc4030_dma_write, dmas[0],
-                   qdev_get_gpio_in(rc4030, 5), &esp_reset, &dma_enable);
+    dev = qdev_create(NULL, TYPE_ESP);
+    sysbus_esp = ESP_STATE(dev);
+    esp = &sysbus_esp->esp;
+    esp->dma_memory_read = rc4030_dma_read;
+    esp->dma_memory_write = rc4030_dma_write;
+    esp->dma_opaque = dmas[0];
+    sysbus_esp->it_shift = 0;
+    /* XXX for now until rc4030 has been changed to use DMA enable signal */
+    esp->dma_enabled = 1;
+    qdev_init_nofail(dev);
+
+    sysbus = SYS_BUS_DEVICE(dev);
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 5));
+    sysbus_mmio_map(sysbus, 0, 0x80002000);
+
     scsi_bus_legacy_handle_cmdline(&esp->bus);
 
     /* Floppy */
-- 
2.11.0

Re: [Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Paolo Bonzini]

On 13/06/2018 11:47, Mark Cave-Ayland wrote:
> +    dev = qdev_create(NULL, TYPE_ESP);
> +    sysbus_esp = ESP_STATE(dev);
> +    esp = &sysbus_esp->esp;
> +    esp->dma_memory_read = rc4030_dma_read;
> +    esp->dma_memory_write = rc4030_dma_write;
> +    esp->dma_opaque = dmas[0];

Poking at the functions here is a bit ugly, and it's the last user of
rc4030_dma_{read,write}.  It would be nicer if ESP could get a memory
region like it's done a bit above for the NIC.  I guess it's not a big
deal, but perhaps there could be a TODO comment.

I'm mostly mentioning this because Hervé is copied and because SPARC DMA
has the same issue of using function pointers instead of an IOMMU memory
region...

Thanks,

Paolo

> +    sysbus_esp->it_shift = 0;
> +    /* XXX for now until rc4030 has been changed to use DMA enable signal */
> +    esp->dma_enabled = 1;
> +    qdev_init_nofail(dev);
> +
> +    sysbus = SYS_BUS_DEVICE(dev);
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 5));
> +    sysbus_mmio_map(sysbus, 0, 0x80002000);
> +

Re: [Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Mark Cave-Ayland]

On 13/06/18 11:06, Paolo Bonzini wrote:

> On 13/06/2018 11:47, Mark Cave-Ayland wrote:
>> +    dev = qdev_create(NULL, TYPE_ESP);
>> +    sysbus_esp = ESP_STATE(dev);
>> +    esp = &sysbus_esp->esp;
>> +    esp->dma_memory_read = rc4030_dma_read;
>> +    esp->dma_memory_write = rc4030_dma_write;
>> +    esp->dma_opaque = dmas[0];
> 
> Poking at the functions here is a bit ugly, and it's the last user of
> rc4030_dma_{read,write}.  It would be nicer if ESP could get a memory
> region like it's done a bit above for the NIC.  I guess it's not a big
> deal, but perhaps there could be a TODO comment.

Okay - I'll wait for Hervé to confirm it passes his tests and then see 
if wants a respin with a TODO added (or can add it himself).

> I'm mostly mentioning this because Hervé is copied and because SPARC DMA
> has the same issue of using function pointers instead of an IOMMU memory
> region...

Ahhh this is a fun one :)

As part of the SPARC32 cleanups for the last release there is now a 
proper sun4m IOMMU implementation, but from memory the big issue was 
that the DMA functions need access to the device state to affect the 
transfer.

Check out hw/dma/sparc32_dma.c for some ugly examples: 
espdma_memory_read()/espdma_memory_write() update the DMA address 
pointer register after each read/write, and 
ledma_memory_read()/ledma_memory_write() need to determine if the pcnet 
card has byte-swapping enabled: if so, then don't perform the required 
lance 16-bit swap and if not, do perform the 16-bit swap.

So yes, it's fairly ugly but I can't think of good solution involving 
just IOMMU memory regions.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Paolo Bonzini]

On 13/06/2018 12:36, Mark Cave-Ayland wrote:
> Check out hw/dma/sparc32_dma.c for some ugly examples:
> espdma_memory_read()/espdma_memory_write() update the DMA address
> pointer register after each read/write, and
> ledma_memory_read()/ledma_memory_write() need to determine if the pcnet
> card has byte-swapping enabled: if so, then don't perform the required
> lance 16-bit swap and if not, do perform the 16-bit swap.

Heh, those are disgusting indeed. :)  So I guess it would have to stay,
only MIPS can use the pure MemoryRegion-based approach.

Regarding pcnet, is CSR_BSWP really a no-op on PCI cards?  If not, an
option could be to move that handling to pcnet.c - making the ledma swap
unconditional and removing the do_bswap argument.  The disadvantage is
that SPARC would swap twice, and you'd have to keep the callback because
of s->dmaregs[3], but maybe it's still worthwhile.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Mark Cave-Ayland]

On 13/06/18 12:19, Paolo Bonzini wrote:

> On 13/06/2018 12:36, Mark Cave-Ayland wrote:
>> Check out hw/dma/sparc32_dma.c for some ugly examples:
>> espdma_memory_read()/espdma_memory_write() update the DMA address
>> pointer register after each read/write, and
>> ledma_memory_read()/ledma_memory_write() need to determine if the pcnet
>> card has byte-swapping enabled: if so, then don't perform the required
>> lance 16-bit swap and if not, do perform the 16-bit swap.
> 
> Heh, those are disgusting indeed. :)  So I guess it would have to stay,
> only MIPS can use the pure MemoryRegion-based approach.

The only option I can think of is inserting an AddressSpace between the 
esp/ledma device and the IOMMU AddressSpace which intercepts the DMA 
request (addr, len, direction).

I can then grab a reference to the device from the MemoryRegion opaque, 
perform the magic, and then manually invoke address_space_rw() on iommu_as.

Is there a hook somewhere in the memory API that could allow me to do this?

> Regarding pcnet, is CSR_BSWP really a no-op on PCI cards?  If not, an
> option could be to move that handling to pcnet.c - making the ledma swap
> unconditional and removing the do_bswap argument.  The disadvantage is
> that SPARC would swap twice, and you'd have to keep the callback because
> of s->dmaregs[3], but maybe it's still worthwhile.

Hmmm good question. If we can intercept the request above, that would be 
my preferred option as something tells it me it might be useful for 
other similar situations.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 1/2] hw/mips/jazz: create ESP device directly via qdev

[Paolo Bonzini]

On 13/06/2018 15:06, Mark Cave-Ayland wrote:
>>
>> Heh, those are disgusting indeed. :)  So I guess it would have to stay,
>> only MIPS can use the pure MemoryRegion-based approach.
> 
> The only option I can think of is inserting an AddressSpace between the
> esp/ledma device and the IOMMU AddressSpace which intercepts the DMA
> request (addr, len, direction).
> 
> I can then grab a reference to the device from the MemoryRegion opaque,
> perform the magic, and then manually invoke address_space_rw() on iommu_as.
> 
> Is there a hook somewhere in the memory API that could allow me to do this?

No, I don't think so.  Only MMIO regions intercept reads/writes, and
they only do it at 1/2/4/8 byte granularity.

>> Regarding pcnet, is CSR_BSWP really a no-op on PCI cards?  If not, an
>> option could be to move that handling to pcnet.c - making the ledma swap
>> unconditional and removing the do_bswap argument.  The disadvantage is
>> that SPARC would swap twice, and you'd have to keep the callback because
>> of s->dmaregs[3], but maybe it's still worthwhile.
> 
> Hmmm good question. If we can intercept the request above, that would be
> my preferred option as something tells it me it might be useful for
> other similar situations.

Yeah, it wouldn't be a great improvement, but there would be the benefit
of more accurate emulation.

Paolo

[Qemu-devel] [PATCH 2/2] esp: remove legacy esp_init() function

[Mark Cave-Ayland]

Remove the legacy esp_init() function now that there are no more remaining
users.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c         | 30 ------------------------------
 include/hw/scsi/esp.h |  5 -----
 2 files changed, 35 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9ed9727744..630d923623 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -619,36 +619,6 @@ static const MemoryRegionOps sysbus_esp_mem_ops = {
     .valid.accepts = esp_mem_accepts,
 };
 
-ESPState *esp_init(hwaddr espaddr, int it_shift,
-                   ESPDMAMemoryReadWriteFunc dma_memory_read,
-                   ESPDMAMemoryReadWriteFunc dma_memory_write,
-                   void *dma_opaque, qemu_irq irq, qemu_irq *reset,
-                   qemu_irq *dma_enable)
-{
-    DeviceState *dev;
-    SysBusDevice *s;
-    SysBusESPState *sysbus;
-    ESPState *esp;
-
-    dev = qdev_create(NULL, TYPE_ESP);
-    sysbus = ESP_STATE(dev);
-    esp = &sysbus->esp;
-    esp->dma_memory_read = dma_memory_read;
-    esp->dma_memory_write = dma_memory_write;
-    esp->dma_opaque = dma_opaque;
-    sysbus->it_shift = it_shift;
-    /* XXX for now until rc4030 has been changed to use DMA enable signal */
-    esp->dma_enabled = 1;
-    qdev_init_nofail(dev);
-    s = SYS_BUS_DEVICE(dev);
-    sysbus_connect_irq(s, 0, irq);
-    sysbus_mmio_map(s, 0, espaddr);
-    *reset = qdev_get_gpio_in(dev, 0);
-    *dma_enable = qdev_get_gpio_in(dev, 1);
-
-    return esp;
-}
-
 static const struct SCSIBusInfo esp_scsi_info = {
     .tcq = false,
     .max_target = ESP_MAX_DEVS,
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 93fdaced67..682a0d2de0 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -131,11 +131,6 @@ typedef struct {
 #define TCHI_FAS100A 0x4
 #define TCHI_AM53C974 0x12
 
-ESPState *esp_init(hwaddr espaddr, int it_shift,
-                   ESPDMAMemoryReadWriteFunc dma_memory_read,
-                   ESPDMAMemoryReadWriteFunc dma_memory_write,
-                   void *dma_opaque, qemu_irq irq, qemu_irq *reset,
-                   qemu_irq *dma_enable);
 void esp_dma_enable(ESPState *s, int irq, int level);
 void esp_request_cancelled(SCSIRequest *req);
 void esp_command_complete(SCSIRequest *req, uint32_t status, size_t resid);
-- 
2.11.0

Re: [Qemu-devel] [PATCH 0/2] scsi: remove legacy esp_init() function

[Mark Cave-Ayland]

On 13/06/18 10:47, Mark Cave-Ayland wrote:

> Something else that came out of reviewing Laurent's q800 patchset: after my
> SPARC cleanups last year, MIPS Jazz is the last remaining user of the legacy
> esp_init() function.
> 
> Patch 1 switches mips_jazz_init() over to create the ESP device directly
> via qdev. Please note that I do not have any MIPS jazz images and so this
> is compile-tested only (although passes "make check") and needs an ACK from
> someone (Hervé?).
> 
> Now that the last user of esp_init() is gone, patch 2 removes the legacy
> function and its associated header.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>    hw/mips/jazz: create ESP device directly via qdev
>    esp: remove legacy esp_init() function
> 
>   hw/mips/mips_jazz.c   | 19 ++++++++++++++++---
>   hw/scsi/esp.c         | 30 ------------------------------
>   include/hw/scsi/esp.h |  5 -----
>   3 files changed, 16 insertions(+), 38 deletions(-)

Ping? Is anyone able to test the MIPS jazz board with these patches at all?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/2] scsi: remove legacy esp_init() function

[Hervé Poussineau]

Le 27/06/2018 à 22:01, Mark Cave-Ayland a écrit :
> On 13/06/18 10:47, Mark Cave-Ayland wrote:
> 
>> Something else that came out of reviewing Laurent's q800 patchset: after my
>> SPARC cleanups last year, MIPS Jazz is the last remaining user of the legacy
>> esp_init() function.
>>
>> Patch 1 switches mips_jazz_init() over to create the ESP device directly
>> via qdev. Please note that I do not have any MIPS jazz images and so this
>> is compile-tested only (although passes "make check") and needs an ACK from
>> someone (Hervé?).
>>
>> Now that the last user of esp_init() is gone, patch 2 removes the legacy
>> function and its associated header.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>>
>> Mark Cave-Ayland (2):
>>    hw/mips/jazz: create ESP device directly via qdev
>>    esp: remove legacy esp_init() function
>>
>>   hw/mips/mips_jazz.c   | 19 ++++++++++++++++---
>>   hw/scsi/esp.c         | 30 ------------------------------
>>   include/hw/scsi/esp.h |  5 -----
>>   3 files changed, 16 insertions(+), 38 deletions(-)
> 
> Ping? Is anyone able to test the MIPS jazz board with these patches at all?

Tested-by: Hervé Poussineau <hpoussin@reactos.org>

Re: [Qemu-devel] [PATCH 0/2] scsi: remove legacy esp_init() function

[Paolo Bonzini]

On 28/06/2018 07:58, Hervé Poussineau wrote:
> Le 27/06/2018 à 22:01, Mark Cave-Ayland a écrit :
>> On 13/06/18 10:47, Mark Cave-Ayland wrote:
>>
>>> Something else that came out of reviewing Laurent's q800 patchset:
>>> after my
>>> SPARC cleanups last year, MIPS Jazz is the last remaining user of the
>>> legacy
>>> esp_init() function.
>>>
>>> Patch 1 switches mips_jazz_init() over to create the ESP device directly
>>> via qdev. Please note that I do not have any MIPS jazz images and so
>>> this
>>> is compile-tested only (although passes "make check") and needs an
>>> ACK from
>>> someone (Hervé?).
>>>
>>> Now that the last user of esp_init() is gone, patch 2 removes the legacy
>>> function and its associated header.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>>
>>> Mark Cave-Ayland (2):
>>>    hw/mips/jazz: create ESP device directly via qdev
>>>    esp: remove legacy esp_init() function
>>>
>>>   hw/mips/mips_jazz.c   | 19 ++++++++++++++++---
>>>   hw/scsi/esp.c         | 30 ------------------------------
>>>   include/hw/scsi/esp.h |  5 -----
>>>   3 files changed, 16 insertions(+), 38 deletions(-)
>>
>> Ping? Is anyone able to test the MIPS jazz board with these patches at
>> all?
> 
> Tested-by: Hervé Poussineau <hpoussin@reactos.org>
> 
> 

Queued, thanks.

Paolo