[Qemu-devel] [PATCH 0/3] via-ide clean ups and native mode

[Qemu-devel] [PATCH 0/3] via-ide clean ups and native mode

[BALATON Zoltan]

Hello,

This implements native PCI ATA mode for via-ida device model which is
what most guests expect and want to use instead of the current legacy
compatibility mode which nothing seems to depend on. Also rename some
functions to via_ide to make this device truely independent from the
VIA 82C686B chip as it can be used independently as well.

This is on top of my previous CMD646 cleanup series.

Regards,
BALATON Zoltan

BALATON Zoltan (3):
  ide/via: Remove vt82c686b_init_ports() function
  ide/via: Rename functions to match device name
  ide/via: Implement and use native PCI IDE mode

 hw/ide/via.c            | 87 ++++++++++++++++++++++++++++++-------------------
 hw/mips/mips_fulong2e.c |  2 +-
 include/hw/ide.h        |  2 +-
 3 files changed, 55 insertions(+), 36 deletions(-)

-- 
2.13.7

[Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode

[BALATON Zoltan]

The device was initialised in ISA compatibility mode and native PCI
IDE mode was not implemented but no clents are known to need ISA mode
but to the contrary, most clients that want to switch to and use
device in native PCI IDE mode. Therefore implement native PCI mode and
switch default to that.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 14 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index c6e43a8812..ac9385228c 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -32,15 +32,6 @@
 #include "hw/ide/pci.h"
 #include "trace.h"
 
-static const struct {
-    int iobase;
-    int iobase2;
-    int isairq;
-} port_info[] = {
-    {0x1f0, 0x3f6, 14},
-    {0x170, 0x376, 15},
-};
-
 static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
 {
@@ -110,6 +101,23 @@ static void bmdma_setup_bar(PCIIDEState *d)
     }
 }
 
+static void via_ide_set_irq(void *opaque, int n, int level)
+{
+    PCIDevice *d = PCI_DEVICE(opaque);
+
+    if (level) {
+        d->config[0x70 + n * 8] |= 0x80;
+    } else {
+        d->config[0x70 + n * 8] &= ~0x80;
+    }
+
+    level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
+    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
+    if (n) {
+        qemu_set_irq(isa_get_irq(NULL, n), level);
+    }
+}
+
 static void via_ide_reset(void *opaque)
 {
     PCIIDEState *d = opaque;
@@ -121,7 +129,7 @@ static void via_ide_reset(void *opaque)
         ide_bus_reset(&d->bus[i]);
     }
 
-    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_WAIT);
+    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
 
@@ -158,10 +166,28 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     uint8_t *pci_conf = dev->config;
     int i;
 
-    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
+    pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
+    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
 
     qemu_register_reset(via_ide_reset, d);
+
+    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
+                          &d->bus[0], "via-ide0-data", 8);
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
+
+    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
+                          &d->bus[0], "via-ide0-cmd", 4);
+    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
+
+    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
+                          &d->bus[1], "via-ide1-data", 8);
+    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
+
+    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
+                          &d->bus[1], "via-ide1-cmd", 4);
+    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
+
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
@@ -169,9 +195,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
 
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                        port_info[i].iobase2);
-        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+        ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
-- 
2.13.7

Re: [Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode

[BALATON Zoltan]

On Tue, 22 Jan 2019, BALATON Zoltan wrote:
> The device was initialised in ISA compatibility mode and native PCI
> IDE mode was not implemented but no clents are known to need ISA mode
> but to the contrary, most clients that want to switch to and use
> device in native PCI IDE mode. Therefore implement native PCI mode and
> switch default to that.

There are some typos in here so here's a fixed, reworded one:

This device only implemented ISA compatibility mode and native PCI IDE 
mode was missing but no clients actually need ISA mode but to the 
contrary, they usually want to switch to and use device in native PCI IDE 
mode. Therefore implement native PCI mode and switch default to that.

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/ide/via.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index c6e43a8812..ac9385228c 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -32,15 +32,6 @@
> #include "hw/ide/pci.h"
> #include "trace.h"
>
> -static const struct {
> -    int iobase;
> -    int iobase2;
> -    int isairq;
> -} port_info[] = {
> -    {0x1f0, 0x3f6, 14},
> -    {0x170, 0x376, 15},
> -};
> -
> static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                            unsigned size)
> {
> @@ -110,6 +101,23 @@ static void bmdma_setup_bar(PCIIDEState *d)
>     }
> }
>
> +static void via_ide_set_irq(void *opaque, int n, int level)
> +{
> +    PCIDevice *d = PCI_DEVICE(opaque);
> +
> +    if (level) {
> +        d->config[0x70 + n * 8] |= 0x80;
> +    } else {
> +        d->config[0x70 + n * 8] &= ~0x80;
> +    }
> +
> +    level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
> +    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
> +    if (n) {
> +        qemu_set_irq(isa_get_irq(NULL, n), level);
> +    }
> +}
> +
> static void via_ide_reset(void *opaque)
> {
>     PCIIDEState *d = opaque;
> @@ -121,7 +129,7 @@ static void via_ide_reset(void *opaque)
>         ide_bus_reset(&d->bus[i]);
>     }
>
> -    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_WAIT);
> +    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
>     pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>                  PCI_STATUS_DEVSEL_MEDIUM);
>
> @@ -158,10 +166,28 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>     uint8_t *pci_conf = dev->config;
>     int i;
>
> -    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
> +    pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
>     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
> +    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
>
>     qemu_register_reset(via_ide_reset, d);
> +
> +    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
> +                          &d->bus[0], "via-ide0-data", 8);
> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
> +
> +    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[0], "via-ide0-cmd", 4);
> +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
> +
> +    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
> +                          &d->bus[1], "via-ide1-data", 8);
> +    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
> +
> +    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[1], "via-ide1-cmd", 4);
> +    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
> +
>     bmdma_setup_bar(d);
>     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>
> @@ -169,9 +195,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>
>     for (i = 0; i < 2; i++) {
>         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> -        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> -                        port_info[i].iobase2);
> -        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
> +        ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
>
>         bmdma_init(&d->bus[i], &d->bmdma[i], d);
>         d->bmdma[i].bus = &d->bus[i];
>

Re: [Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode

[John Snow]

On 1/22/19 7:39 AM, BALATON Zoltan wrote:
> The device was initialised in ISA compatibility mode and native PCI
> IDE mode was not implemented but no clents are known to need ISA mode
> but to the contrary, most clients that want to switch to and use
> device in native PCI IDE mode. Therefore implement native PCI mode and
> switch default to that.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ide/via.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index c6e43a8812..ac9385228c 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -32,15 +32,6 @@
>  #include "hw/ide/pci.h"
>  #include "trace.h"
>  
> -static const struct {
> -    int iobase;
> -    int iobase2;
> -    int isairq;
> -} port_info[] = {
> -    {0x1f0, 0x3f6, 14},
> -    {0x170, 0x376, 15},
> -};
> -
>  static uint64_t bmdma_read(void *opaque, hwaddr addr,
>                             unsigned size)
>  {
> @@ -110,6 +101,23 @@ static void bmdma_setup_bar(PCIIDEState *d)
>      }
>  }
>  
> +static void via_ide_set_irq(void *opaque, int n, int level)
> +{
> +    PCIDevice *d = PCI_DEVICE(opaque);
> +
> +    if (level) {
> +        d->config[0x70 + n * 8] |= 0x80;
> +    } else {
> +        d->config[0x70 + n * 8] &= ~0x80;
> +    }
> +
> +    level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
> +    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
> +    if (n) {
> +        qemu_set_irq(isa_get_irq(NULL, n), level);
> +    }
> +}
> +
>  static void via_ide_reset(void *opaque)
>  {
>      PCIIDEState *d = opaque;
> @@ -121,7 +129,7 @@ static void via_ide_reset(void *opaque)
>          ide_bus_reset(&d->bus[i]);
>      }
>  
> -    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_WAIT);
> +    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
>      pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>                   PCI_STATUS_DEVSEL_MEDIUM);
>  
> @@ -158,10 +166,28 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>      uint8_t *pci_conf = dev->config;
>      int i;
>  
> -    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
> +    pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
>      pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
> +    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
>  
>      qemu_register_reset(via_ide_reset, d);
> +
> +    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
> +                          &d->bus[0], "via-ide0-data", 8);
> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
> +
> +    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[0], "via-ide0-cmd", 4);
> +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
> +
> +    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
> +                          &d->bus[1], "via-ide1-data", 8);
> +    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
> +
> +    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
> +                          &d->bus[1], "via-ide1-cmd", 4);
> +    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
> +
>      bmdma_setup_bar(d);
>      pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>  
> @@ -169,9 +195,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>  
>      for (i = 0; i < 2; i++) {
>          ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
> -        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
> -                        port_info[i].iobase2);
> -        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
> +        ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
>  
>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>          d->bmdma[i].bus = &d->bus[i];
> 

I guess this is technically an external change in behavior... I have no
real read on if this will break anything for anyone, or if anyone was
even using it.

Any thoughts on why it's okay to switch?

Re: [Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode

[BALATON Zoltan]

On Wed, 23 Jan 2019, John Snow wrote:
> On 1/22/19 7:39 AM, BALATON Zoltan wrote:
>> The device was initialised in ISA compatibility mode and native PCI
>> IDE mode was not implemented but no clents are known to need ISA mode
>> but to the contrary, most clients that want to switch to and use
>> device in native PCI IDE mode. Therefore implement native PCI mode and
>> switch default to that.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ide/via.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 38 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index c6e43a8812..ac9385228c 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -32,15 +32,6 @@
>>  #include "hw/ide/pci.h"
>>  #include "trace.h"
>>
>> -static const struct {
>> -    int iobase;
>> -    int iobase2;
>> -    int isairq;
>> -} port_info[] = {
>> -    {0x1f0, 0x3f6, 14},
>> -    {0x170, 0x376, 15},
>> -};
>> -
>>  static uint64_t bmdma_read(void *opaque, hwaddr addr,
>>                             unsigned size)
>>  {
>> @@ -110,6 +101,23 @@ static void bmdma_setup_bar(PCIIDEState *d)
>>      }
>>  }
>>
>> +static void via_ide_set_irq(void *opaque, int n, int level)
>> +{
>> +    PCIDevice *d = PCI_DEVICE(opaque);
>> +
>> +    if (level) {
>> +        d->config[0x70 + n * 8] |= 0x80;
>> +    } else {
>> +        d->config[0x70 + n * 8] &= ~0x80;
>> +    }
>> +
>> +    level = (d->config[0x70] & 0x80) || (d->config[0x78] & 0x80);
>> +    n = pci_get_byte(d->config + PCI_INTERRUPT_LINE);
>> +    if (n) {
>> +        qemu_set_irq(isa_get_irq(NULL, n), level);
>> +    }
>> +}
>> +
>>  static void via_ide_reset(void *opaque)
>>  {
>>      PCIIDEState *d = opaque;
>> @@ -121,7 +129,7 @@ static void via_ide_reset(void *opaque)
>>          ide_bus_reset(&d->bus[i]);
>>      }
>>
>> -    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_WAIT);
>> +    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_IO | PCI_COMMAND_WAIT);
>>      pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
>>                   PCI_STATUS_DEVSEL_MEDIUM);
>>
>> @@ -158,10 +166,28 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>>      uint8_t *pci_conf = dev->config;
>>      int i;
>>
>> -    pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
>> +    pci_config_set_prog_interface(pci_conf, 0x8f); /* native PCI ATA mode */
>>      pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
>> +    dev->wmask[PCI_INTERRUPT_LINE] = 0xf;
>>
>>      qemu_register_reset(via_ide_reset, d);
>> +
>> +    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
>> +                          &d->bus[0], "via-ide0-data", 8);
>> +    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
>> +
>> +    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
>> +                          &d->bus[0], "via-ide0-cmd", 4);
>> +    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
>> +
>> +    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
>> +                          &d->bus[1], "via-ide1-data", 8);
>> +    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
>> +
>> +    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
>> +                          &d->bus[1], "via-ide1-cmd", 4);
>> +    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
>> +
>>      bmdma_setup_bar(d);
>>      pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>>
>> @@ -169,9 +195,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>>
>>      for (i = 0; i < 2; i++) {
>>          ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
>> -        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
>> -                        port_info[i].iobase2);
>> -        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
>> +        ide_init2(&d->bus[i], qemu_allocate_irq(via_ide_set_irq, d, i));
>>
>>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>          d->bmdma[i].bus = &d->bus[i];
>>
>
> I guess this is technically an external change in behavior... I have no
> real read on if this will break anything for anyone, or if anyone was
> even using it.

Currently nothing but the mips_fulong2e board seems to use this device and 
I don't think there's anything even on that board that would depend on (or 
would work with) legacy only IDE mode of this device but I could not find 
a test image to try. That board seems to be quite unfinished, I've tried 
to get it in better shape but haven't succeeded yet. So I don't think this 
change in the IDE device would break anything for anyone as it does not 
seem to be usable yet but I plan to use this IDE part in subsequent 
patches and PCI native mode works better.

> Any thoughts on why it's okay to switch?

As said above it's unlikely anyone would depend on it now and all drivers 
I know about prefer native mode anyway so likely it would work better 
after this change. If not and it turns out someone is using the current 
behaviour I can look at implementing switching between the two modes but 
that would be more difficult (the ISA ports would need to be mapped and 
unmapped based on bits in PCI_PROG_INTERFACE but there's no API to do this 
currently, ide/core.c only has ide_init_ioport to add mapping but not the 
counterpart to remove it; this could be implemented but unless found to be 
needed it probably does not worth it). So I suggest to switch based on 
that this is a quite unused part that's not likely to break anything and 
if it still found to break something I'll look at fixing it or it could be 
reverted, but I don't want to spend time now on something that's not 
actually used.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode

[BALATON Zoltan]

On Thu, 24 Jan 2019, BALATON Zoltan wrote:
> On Wed, 23 Jan 2019, John Snow wrote:
>> I guess this is technically an external change in behavior... I have no
>> real read on if this will break anything for anyone, or if anyone was
>> even using it.
>
> Currently nothing but the mips_fulong2e board seems to use this device and I 
> don't think there's anything even on that board that would depend on (or 
> would work with) legacy only IDE mode of this device but I could not find a 
> test image to try. That board seems to be quite unfinished, I've tried to get 
> it in better shape but haven't succeeded yet. So I don't think this change in 
> the IDE device would break anything for anyone as it does not seem to be 
> usable yet but I plan to use this IDE part in subsequent patches and PCI 
> native mode works better.
>
>> Any thoughts on why it's okay to switch?
>
> As said above it's unlikely anyone would depend on it now and all drivers I 
> know about prefer native mode anyway so likely it would work better after 
> this change. If not and it turns out someone is using the current behaviour I 
> can look at implementing switching between the two modes but that would be 
> more difficult (the ISA ports would need to be mapped and unmapped based on 
> bits in PCI_PROG_INTERFACE but there's no API to do this currently, 
> ide/core.c only has ide_init_ioport to add mapping but not the counterpart to 
> remove it; this could be implemented but unless found to be needed it 
> probably does not worth it). So I suggest to switch based on that this is a 
> quite unused part that's not likely to break anything and if it still found 
> to break something I'll look at fixing it or it could be reverted, but I 
> don't want to spend time now on something that's not actually used.

I'd appreciated if this could be included now based on the above as I have 
some pathces in the works that will need this, unless there's a strong 
reason to not apply it.

Thank you,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 3/3] ide/via: Implement and use native PCI IDE mode

[John Snow]

On 1/25/19 7:25 AM, BALATON Zoltan wrote:
> On Thu, 24 Jan 2019, BALATON Zoltan wrote:
>> On Wed, 23 Jan 2019, John Snow wrote:
>>> I guess this is technically an external change in behavior... I have no
>>> real read on if this will break anything for anyone, or if anyone was
>>> even using it.
>>
>> Currently nothing but the mips_fulong2e board seems to use this device
>> and I don't think there's anything even on that board that would
>> depend on (or would work with) legacy only IDE mode of this device but
>> I could not find a test image to try. That board seems to be quite
>> unfinished, I've tried to get it in better shape but haven't succeeded
>> yet. So I don't think this change in the IDE device would break
>> anything for anyone as it does not seem to be usable yet but I plan to
>> use this IDE part in subsequent patches and PCI native mode works better.
>>
>>> Any thoughts on why it's okay to switch?
>>
>> As said above it's unlikely anyone would depend on it now and all
>> drivers I know about prefer native mode anyway so likely it would work
>> better after this change. If not and it turns out someone is using the
>> current behaviour I can look at implementing switching between the two
>> modes but that would be more difficult (the ISA ports would need to be
>> mapped and unmapped based on bits in PCI_PROG_INTERFACE but there's no
>> API to do this currently, ide/core.c only has ide_init_ioport to add
>> mapping but not the counterpart to remove it; this could be
>> implemented but unless found to be needed it probably does not worth
>> it). So I suggest to switch based on that this is a quite unused part
>> that's not likely to break anything and if it still found to break
>> something I'll look at fixing it or it could be reverted, but I don't
>> want to spend time now on something that's not actually used.
> 
> I'd appreciated if this could be included now based on the above as I
> have some pathces in the works that will need this, unless there's a
> strong reason to not apply it.
> 
> Thank you,
> BALATON Zoltan

I'll stage it. If anyone disagrees we can roll it back before release if
we are breaking legacy behavior, but I really strongly suspect nobody is
relying on this in any kind of production environment, so I'll take the
risk to just merge the improvement.

--js

[Qemu-devel] [PATCH 1/3] ide/via: Remove vt82c686b_init_ports() function

[BALATON Zoltan]

This function is only called once from vt82c686b_ide_realize() and its
content is simple enough to not need a separate function but be
included in realize directly (as done in other IDE models except PIIX
currently).

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 987d99c5ec..46cac7b8d6 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -32,6 +32,15 @@
 #include "hw/ide/pci.h"
 #include "trace.h"
 
+static const struct {
+    int iobase;
+    int iobase2;
+    int isairq;
+} port_info[] = {
+    {0x1f0, 0x3f6, 14},
+    {0x170, 0x376, 15},
+};
+
 static uint64_t bmdma_read(void *opaque, hwaddr addr,
                            unsigned size)
 {
@@ -143,34 +152,12 @@ static void via_reset(void *opaque)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
-static void vt82c686b_init_ports(PCIIDEState *d) {
-    static const struct {
-        int iobase;
-        int iobase2;
-        int isairq;
-    } port_info[] = {
-        {0x1f0, 0x3f6, 14},
-        {0x170, 0x376, 15},
-    };
-    int i;
-
-    for (i = 0; i < 2; i++) {
-        ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
-        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
-                        port_info[i].iobase2);
-        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
-
-        bmdma_init(&d->bus[i], &d->bmdma[i], d);
-        d->bmdma[i].bus = &d->bus[i];
-        ide_register_restart_cb(&d->bus[i]);
-    }
-}
-
 /* via ide func */
 static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
 {
     PCIIDEState *d = PCI_IDE(dev);
     uint8_t *pci_conf = dev->config;
+    int i;
 
     pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
@@ -181,7 +168,16 @@ static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
 
     vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
 
-    vt82c686b_init_ports(d);
+    for (i = 0; i < 2; i++) {
+        ide_bus_new(&d->bus[i], sizeof(d->bus[i]), DEVICE(d), i, 2);
+        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+                        port_info[i].iobase2);
+        ide_init2(&d->bus[i], isa_get_irq(NULL, port_info[i].isairq));
+
+        bmdma_init(&d->bus[i], &d->bmdma[i], d);
+        d->bmdma[i].bus = &d->bus[i];
+        ide_register_restart_cb(&d->bus[i]);
+    }
 }
 
 static void vt82c686b_ide_exitfn(PCIDevice *dev)
-- 
2.13.7

[Qemu-devel] [PATCH 2/3] ide/via: Rename functions to match device name

[BALATON Zoltan]

The device is called via-ide and the modelled IDE controller is not
specific to 82C686B but is also usable independently. Therefore, change
function name prefixes accordingly to match device name.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c            | 15 +++++++--------
 hw/mips/mips_fulong2e.c |  2 +-
 include/hw/ide.h        |  2 +-
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 46cac7b8d6..c6e43a8812 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -110,7 +110,7 @@ static void bmdma_setup_bar(PCIIDEState *d)
     }
 }
 
-static void via_reset(void *opaque)
+static void via_ide_reset(void *opaque)
 {
     PCIIDEState *d = opaque;
     PCIDevice *pd = PCI_DEVICE(d);
@@ -152,8 +152,7 @@ static void via_reset(void *opaque)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
-/* via ide func */
-static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
+static void via_ide_realize(PCIDevice *dev, Error **errp)
 {
     PCIIDEState *d = PCI_IDE(dev);
     uint8_t *pci_conf = dev->config;
@@ -162,7 +161,7 @@ static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
     pci_config_set_prog_interface(pci_conf, 0x8a); /* legacy ATA mode */
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
 
-    qemu_register_reset(via_reset, d);
+    qemu_register_reset(via_ide_reset, d);
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
@@ -180,7 +179,7 @@ static void vt82c686b_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void vt82c686b_ide_exitfn(PCIDevice *dev)
+static void via_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = PCI_IDE(dev);
     unsigned i;
@@ -191,7 +190,7 @@ static void vt82c686b_ide_exitfn(PCIDevice *dev)
     }
 }
 
-void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
+void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
 {
     PCIDevice *dev;
 
@@ -204,8 +203,8 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->realize = vt82c686b_ide_realize;
-    k->exit = vt82c686b_ide_exitfn;
+    k->realize = via_ide_realize;
+    k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_IDE;
     k->revision = 0x06;
diff --git a/hw/mips/mips_fulong2e.c b/hw/mips/mips_fulong2e.c
index 2fbba32c48..42d09f6892 100644
--- a/hw/mips/mips_fulong2e.c
+++ b/hw/mips/mips_fulong2e.c
@@ -249,7 +249,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
 
     ide_drive_get(hd, ARRAY_SIZE(hd));
-    vt82c686b_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
+    via_ide_init(pci_bus, hd, PCI_DEVFN(slot, 1));
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci");
diff --git a/include/hw/ide.h b/include/hw/ide.h
index 3ae087c572..28d8a06439 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -18,7 +18,7 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 int pci_piix3_xen_ide_unplug(DeviceState *dev, bool aux);
-void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
+void via_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
 
 /* ide-mmio.c */
 void mmio_ide_init_drives(DeviceState *dev, DriveInfo *hd0, DriveInfo *hd1);
-- 
2.13.7