[PATCH v2 0/2] Fix via-ide for fuloong2e

[PATCH v2 0/2] Fix via-ide for fuloong2e

[BALATON Zoltan via]

This implements the legacy-mode emulation option for via-ide which is
needed for Linux on fuloong2e. I've tested that the Debian kernel now
finds CD ROM and MorphOS on pegasos2 is not affected by this.

v2 adds review tags and fixes

BALATON Zoltan (1):
  ide: Make room for flags in PCIIDEState and add one for legacy mode

Guenter Roeck (1):
  via-ide: Fix fuloong2e support

 hw/ide/cmd646.c      |  4 ++--
 hw/ide/via.c         | 19 +++++++++++++++++--
 hw/mips/fuloong2e.c  |  4 +++-
 hw/sparc64/sun4u.c   |  2 +-
 include/hw/ide/pci.h |  7 ++++++-
 5 files changed, 29 insertions(+), 7 deletions(-)

-- 
2.21.3

[PATCH v2 2/2] via-ide: Fix fuloong2e support

[BALATON Zoltan via]

From: Guenter Roeck <linux@roeck-us.net>

The IDE legacy mode emulation has been removed in commit 4ea98d317eb
("ide/via: Implement and use native PCI IDE mode") but some Linux
kernels (probably including def_config) require legacy mode on the
Fuloong2e so only emulating native mode did not turn out feasible.
Add property to via-ide model to make the mode configurable, and set
legacy mode for Fuloong2e.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
[balaton: Use bit in flags for property, add comment for missing BAR4]
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Reworded commit message

 hw/ide/via.c        | 19 +++++++++++++++++--
 hw/mips/fuloong2e.c |  4 +++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index be09912b33..7d54d7e829 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -26,6 +26,7 @@
 
 #include "qemu/osdep.h"
 #include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
 #include "sysemu/dma.h"
@@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
                           &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);
+    if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
+        /* Missing BAR4 will make Linux driver fall back to legacy PIO mode */
+        bmdma_setup_bar(d);
+        pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
+    }
 
     qdev_init_gpio_in(ds, via_ide_set_irq, 2);
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
+        if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
+            ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
+                            i ? 0x376 : 0x3f6);
+        }
         ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
@@ -210,6 +218,12 @@ static void via_ide_exitfn(PCIDevice *dev)
     }
 }
 
+static Property via_ide_properties[] = {
+    DEFINE_PROP_BIT("legacy_mode", PCIIDEState, flags, PCI_IDE_LEGACY_MODE,
+                    false),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void via_ide_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -223,6 +237,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_VIA_IDE;
     k->revision = 0x06;
     k->class_id = PCI_CLASS_STORAGE_IDE;
+    device_class_set_props(dc, via_ide_properties);
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
 
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 45c596f4fe..f0733e87b7 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -253,7 +253,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     /* Super I/O */
     isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
 
-    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
+    dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
+    qdev_prop_set_bit(&dev->qdev, "legacy_mode", true);
+    pci_realize_and_unref(dev, pci_bus, &error_fatal);
     pci_ide_create_devs(dev);
 
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");
-- 
2.21.3

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[Jiaxun Yang]

在 2020/12/28 上午6:13, BALATON Zoltan 写道:
> From: Guenter Roeck <linux@roeck-us.net>
>
> The IDE legacy mode emulation has been removed in commit 4ea98d317eb
> ("ide/via: Implement and use native PCI IDE mode") but some Linux
> kernels (probably including def_config) require legacy mode on the
> Fuloong2e so only emulating native mode did not turn out feasible.
> Add property to via-ide model to make the mode configurable, and set
> legacy mode for Fuloong2e.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> [balaton: Use bit in flags for property, add comment for missing BAR4]
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

> ---
> v2: Reworded commit message
>
>

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[Mark Cave-Ayland]

On 27/12/2020 22:13, BALATON Zoltan via wrote:

> From: Guenter Roeck <linux@roeck-us.net>
> 
> The IDE legacy mode emulation has been removed in commit 4ea98d317eb
> ("ide/via: Implement and use native PCI IDE mode") but some Linux
> kernels (probably including def_config) require legacy mode on the
> Fuloong2e so only emulating native mode did not turn out feasible.
> Add property to via-ide model to make the mode configurable, and set
> legacy mode for Fuloong2e.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> [balaton: Use bit in flags for property, add comment for missing BAR4]
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Reworded commit message
> 
>   hw/ide/via.c        | 19 +++++++++++++++++--
>   hw/mips/fuloong2e.c |  4 +++-
>   2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index be09912b33..7d54d7e829 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -26,6 +26,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "hw/pci/pci.h"
> +#include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
>   #include "qemu/module.h"
>   #include "sysemu/dma.h"
> @@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>                             &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);
> +    if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
> +        /* Missing BAR4 will make Linux driver fall back to legacy PIO mode */
> +        bmdma_setup_bar(d);
> +        pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
> +    }

Since the default value of the legacy mode parameter is false, then this means the 
device assumes native mode by default. Therefore PCI_CLASS_PROG should be set to 0x8f 
unless legacy mode is being used, in which case it should be 0x8a.

>       qdev_init_gpio_in(ds, via_ide_set_irq, 2);
>       for (i = 0; i < 2; i++) {
>           ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
> +        if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
> +            ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
> +                            i ? 0x376 : 0x3f6);
> +        }

You could actually remove the if() here: PCI configuration always leaves a gap at the 
lower end of IO address space to avoid clashes with legacy ports. Therefore if a 
guest decides to switch to native mode and configure the BAR, it will never clash 
with the default legacy IDE ports. This has the advantage of minimising the parts 
protected by PCI_IDE_LEGACY_MODE whilst also providing the legacy ports if someone 
can devise a method to dynamically switch between compatible and native modes later.

>           ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
>   
>           bmdma_init(&d->bus[i], &d->bmdma[i], d);
> @@ -210,6 +218,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>       }
>   }
>   
> +static Property via_ide_properties[] = {
> +    DEFINE_PROP_BIT("legacy_mode", PCIIDEState, flags, PCI_IDE_LEGACY_MODE,
> +                    false),

The convention for new qdev/QOM properties is that they should use hyphens instead of 
underscores (see the comment underneath object_property_try_add() at 
https://qemu.readthedocs.io/en/latest/devel/qom.html).

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>   static void via_ide_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -223,6 +237,7 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
>       k->device_id = PCI_DEVICE_ID_VIA_IDE;
>       k->revision = 0x06;
>       k->class_id = PCI_CLASS_STORAGE_IDE;
> +    device_class_set_props(dc, via_ide_properties);
>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>   }
>   
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 45c596f4fe..f0733e87b7 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -253,7 +253,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>       /* Super I/O */
>       isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>   
> -    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
> +    dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
> +    qdev_prop_set_bit(&dev->qdev, "legacy_mode", true);
> +    pci_realize_and_unref(dev, pci_bus, &error_fatal);
>       pci_ide_create_devs(dev);
>   
>       pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci");


ATB,

Mark.

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[BALATON Zoltan via]

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

On Mon, 28 Dec 2020, Mark Cave-Ayland wrote:
> On 27/12/2020 22:13, BALATON Zoltan via wrote:
>
>> From: Guenter Roeck <linux@roeck-us.net>
>> 
>> The IDE legacy mode emulation has been removed in commit 4ea98d317eb
>> ("ide/via: Implement and use native PCI IDE mode") but some Linux
>> kernels (probably including def_config) require legacy mode on the
>> Fuloong2e so only emulating native mode did not turn out feasible.
>> Add property to via-ide model to make the mode configurable, and set
>> legacy mode for Fuloong2e.
>> 
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> [balaton: Use bit in flags for property, add comment for missing BAR4]
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Reworded commit message
>>
>>   hw/ide/via.c        | 19 +++++++++++++++++--
>>   hw/mips/fuloong2e.c |  4 +++-
>>   2 files changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index be09912b33..7d54d7e829 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -26,6 +26,7 @@
>>     #include "qemu/osdep.h"
>>   #include "hw/pci/pci.h"
>> +#include "hw/qdev-properties.h"
>>   #include "migration/vmstate.h"
>>   #include "qemu/module.h"
>>   #include "sysemu/dma.h"
>> @@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error 
>> **errp)
>>                             &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);
>> +    if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
>> +        /* Missing BAR4 will make Linux driver fall back to legacy PIO 
>> mode */
>> +        bmdma_setup_bar(d);
>> +        pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, 
>> &d->bmdma_bar);
>> +    }
>
> Since the default value of the legacy mode parameter is false, then this 
> means the device assumes native mode by default. Therefore PCI_CLASS_PROG 
> should be set to 0x8f unless legacy mode is being used, in which case it 
> should be 0x8a.

I think this casued problems before because if it's not set to 0x8a 
(legacy) at start then guests may assume it's already switched to native 
mode by firmware and won't program the BARs and it will not work. This 
way, even if it looks odd all guests I've tested work so I don't want to 
touch this, because I don't want to test all guests again.

Keep in mind we're not fully emulating this device so not every 
combination that may work on real hardware work in this model. We really 
either only emulate "half-native" mode (i.e. native mode with ISA IRQs) 
that's needed for pegasos2 guests or now again only emulate legacy mode if 
property is set for Linux on fuloong2e. (My original patch emulated 
half-native and native mode instead of legacy thinking that Linux on 
fuloong2e would adapt.) All other combinations, including switching 
between these two is expected to not work which is due to QEMU limitations 
as you've also discovered now. I think this is still an improvement over 
only emulating legacy mode before even if it does not yet fully model the 
chip and I've also cleaned up PCI IDE emulation during implementing this 
so that code can be shared between via-ide, sii3112 and CMD646 without 
much duplication.

>>       qdev_init_gpio_in(ds, via_ide_set_irq, 2);
>>       for (i = 0; i < 2; i++) {
>>           ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
>> +        if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
>> +            ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
>> +                            i ? 0x376 : 0x3f6);
>> +        }
>
> You could actually remove the if() here: PCI configuration always leaves a 
> gap at the lower end of IO address space to avoid clashes with legacy ports. 
> Therefore if a guest decides to switch to native mode and configure the BAR, 
> it will never clash with the default legacy IDE ports. This has the advantage 
> of minimising the parts protected by PCI_IDE_LEGACY_MODE whilst also 
> providing the legacy ports if someone can devise a method to dynamically 
> switch between compatible and native modes later.

I think leaving the legacy ports enabled is a bad idea for at least two 
reasons: 1) It may clash with other io ports on other machines, e.g. I'm 
not sure on PPC where firmware or OS does not expect to see legacy ISA 
ports won't map some io BAR of a PCI card there. 2) If this is left 
enabled there would be two ports poking the same registers so if that does 
not cause a problem by itself, writing to one accidentally (like when 
something is mapped over it) could cause corruption of IDE state so I 
think it's much better to protect this than later trying to debug such 
problems. (You can't get rid of the flag without implementing mode 
switching that needs rewrite of ISA and PCI emulation in QEMU so just get 
over it.)

>>           ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
>>             bmdma_init(&d->bus[i], &d->bmdma[i], d);
>> @@ -210,6 +218,12 @@ static void via_ide_exitfn(PCIDevice *dev)
>>       }
>>   }
>>   +static Property via_ide_properties[] = {
>> +    DEFINE_PROP_BIT("legacy_mode", PCIIDEState, flags, 
>> PCI_IDE_LEGACY_MODE,
>> +                    false),
>
> The convention for new qdev/QOM properties is that they should use hyphens 
> instead of underscores (see the comment underneath object_property_try_add() 
> at https://qemu.readthedocs.io/en/latest/devel/qom.html).

I'll fix this with the comment in the first patch.

Regards,
BALATON Zoltan

>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>>   static void via_ide_class_init(ObjectClass *klass, void *data)
>>   {
>>       DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -223,6 +237,7 @@ static void via_ide_class_init(ObjectClass *klass, void 
>> *data)
>>       k->device_id = PCI_DEVICE_ID_VIA_IDE;
>>       k->revision = 0x06;
>>       k->class_id = PCI_CLASS_STORAGE_IDE;
>> +    device_class_set_props(dc, via_ide_properties);
>>       set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
>>   }
>>   diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 45c596f4fe..f0733e87b7 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -253,7 +253,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, 
>> int slot, qemu_irq intc,
>>       /* Super I/O */
>>       isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
>>   -    dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>> +    dev = pci_new(PCI_DEVFN(slot, 1), "via-ide");
>> +    qdev_prop_set_bit(&dev->qdev, "legacy_mode", true);
>> +    pci_realize_and_unref(dev, pci_bus, &error_fatal);
>>       pci_ide_create_devs(dev);
>>         pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), 
>> "vt82c686b-usb-uhci");
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[Philippe Mathieu-Daudé]

On 12/28/20 9:50 PM, BALATON Zoltan via wrote:
> On Mon, 28 Dec 2020, Mark Cave-Ayland wrote:
>> On 27/12/2020 22:13, BALATON Zoltan via wrote:
>>
>>> From: Guenter Roeck <linux@roeck-us.net>
>>>
>>> The IDE legacy mode emulation has been removed in commit 4ea98d317eb
>>> ("ide/via: Implement and use native PCI IDE mode") but some Linux
>>> kernels (probably including def_config) require legacy mode on the
>>> Fuloong2e so only emulating native mode did not turn out feasible.
>>> Add property to via-ide model to make the mode configurable, and set
>>> legacy mode for Fuloong2e.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> [balaton: Use bit in flags for property, add comment for missing BAR4]
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> v2: Reworded commit message
>>>
>>>   hw/ide/via.c        | 19 +++++++++++++++++--
>>>   hw/mips/fuloong2e.c |  4 +++-
>>>   2 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index be09912b33..7d54d7e829 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -26,6 +26,7 @@
>>>     #include "qemu/osdep.h"
>>>   #include "hw/pci/pci.h"
>>> +#include "hw/qdev-properties.h"
>>>   #include "migration/vmstate.h"
>>>   #include "qemu/module.h"
>>>   #include "sysemu/dma.h"
>>> @@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev,
>>> Error **errp)
>>>                             &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);
>>> +    if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
>>> +        /* Missing BAR4 will make Linux driver fall back to legacy
>>> PIO mode */
>>> +        bmdma_setup_bar(d);
>>> +        pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO,
>>> &d->bmdma_bar);
>>> +    }
>>
>> Since the default value of the legacy mode parameter is false, then
>> this means the device assumes native mode by default. Therefore
>> PCI_CLASS_PROG should be set to 0x8f unless legacy mode is being used,
>> in which case it should be 0x8a.
> 
> I think this casued problems before because if it's not set to 0x8a
> (legacy) at start then guests may assume it's already switched to native
> mode by firmware and won't program the BARs and it will not work. This
> way, even if it looks odd all guests I've tested work so I don't want to
> touch this, because I don't want to test all guests again.

If you can describe on the list how you do your testing (mostly
command line used, where image/demo can be downloaded), we might help
writing an integration test to automate the testing. Don't worry if
it involves using close-source binaries, we'll try to figure out a
way.

Regards,

Phil.

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[BALATON Zoltan via]

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

On Tue, 29 Dec 2020, Philippe Mathieu-Daudé wrote:
>> I think this casued problems before because if it's not set to 0x8a
>> (legacy) at start then guests may assume it's already switched to native
>> mode by firmware and won't program the BARs and it will not work. This
>> way, even if it looks odd all guests I've tested work so I don't want to
>> touch this, because I don't want to test all guests again.
>
> If you can describe on the list how you do your testing (mostly
> command line used, where image/demo can be downloaded), we might help
> writing an integration test to automate the testing. Don't worry if
> it involves using close-source binaries, we'll try to figure out a
> way.

It's documented here:

https://osdn.net/projects/qmiga/wiki/SubprojectPegasos2#h2-Current.20status.20and.20outstanding.20issues

but pegasos2 is not in QEMU master yet, I'm just trying to clean it up and 
submit it so testing likely comes after I have a set of patches to send to 
add pegasos2. The version in qmiga repo is not the latest but one that 
works in itself, my current vt82c686c work is to upstream the vt8231 part. 
After that the rest should be simple: add mv64361 model and pegasos2 board 
code. The only problem I see with those is if it's OK to have board 
needing ROM image or if Mark torpedoes it for some reason :-). For the ROM 
I have plans to use vof (virtual open firmware emulation within QEMU, 
unmerged on qemu-ppc list currently for a while) but I couldn't get to 
that yet. Maybe Linux can be used via -kernel but I think it also needs 
Open Firmware client interface on pegasos2 so it won't work without some 
kind of firmware. (All this is also documented on above page if you scroll 
up.)

(By the way I've created the qmiga.osdn.net project with the intent to 
share work with interested people on these but so far nobody seemed to be 
interested enough to join and help. There were about 2 or 3 people 
interested in ati-vga but got scared away when saw how much work is needed 
yet. Contributions are still welcome otherwise it will take another few 
years to finish.)

Regards,
BALATON Zoltan

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[Mark Cave-Ayland]

On 28/12/2020 20:50, BALATON Zoltan via wrote:

>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>> index be09912b33..7d54d7e829 100644
>>> --- a/hw/ide/via.c
>>> +++ b/hw/ide/via.c
>>> @@ -26,6 +26,7 @@
>>>     #include "qemu/osdep.h"
>>>   #include "hw/pci/pci.h"
>>> +#include "hw/qdev-properties.h"
>>>   #include "migration/vmstate.h"
>>>   #include "qemu/module.h"
>>>   #include "sysemu/dma.h"
>>> @@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
>>>                             &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);
>>> +    if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
>>> +        /* Missing BAR4 will make Linux driver fall back to legacy PIO mode */
>>> +        bmdma_setup_bar(d);
>>> +        pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
>>> +    }
>>
>> Since the default value of the legacy mode parameter is false, then this means the 
>> device assumes native mode by default. Therefore PCI_CLASS_PROG should be set to 
>> 0x8f unless legacy mode is being used, in which case it should be 0x8a.
> 
> I think this casued problems before because if it's not set to 0x8a (legacy) at start 
> then guests may assume it's already switched to native mode by firmware and won't 
> program the BARs and it will not work. This way, even if it looks odd all guests I've 
> tested work so I don't want to touch this, because I don't want to test all guests 
> again.
> 
> Keep in mind we're not fully emulating this device so not every combination that may 
> work on real hardware work in this model. We really either only emulate "half-native" 
> mode (i.e. native mode with ISA IRQs) that's needed for pegasos2 guests or now again 
> only emulate legacy mode if property is set for Linux on fuloong2e. (My original 
> patch emulated half-native and native mode instead of legacy thinking that Linux on 
> fuloong2e would adapt.) All other combinations, including switching between these two 
> is expected to not work which is due to QEMU limitations as you've also discovered 
> now. I think this is still an improvement over only emulating legacy mode before even 
> if it does not yet fully model the chip and I've also cleaned up PCI IDE emulation 
> during implementing this so that code can be shared between via-ide, sii3112 and 
> CMD646 without much duplication.
> 
>>>       qdev_init_gpio_in(ds, via_ide_set_irq, 2);
>>>       for (i = 0; i < 2; i++) {
>>>           ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
>>> +        if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
>>> +            ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
>>> +                            i ? 0x376 : 0x3f6);
>>> +        }
>>
>> You could actually remove the if() here: PCI configuration always leaves a gap at 
>> the lower end of IO address space to avoid clashes with legacy ports. Therefore if 
>> a guest decides to switch to native mode and configure the BAR, it will never clash 
>> with the default legacy IDE ports. This has the advantage of minimising the parts 
>> protected by PCI_IDE_LEGACY_MODE whilst also providing the legacy ports if someone 
>> can devise a method to dynamically switch between compatible and native modes later.
> 
> I think leaving the legacy ports enabled is a bad idea for at least two reasons: 1) 
> It may clash with other io ports on other machines, e.g. I'm not sure on PPC where 
> firmware or OS does not expect to see legacy ISA ports won't map some io BAR of a PCI 
> card there. 2) If this is left enabled there would be two ports poking the same 
> registers so if that does not cause a problem by itself, writing to one accidentally 
> (like when something is mapped over it) could cause corruption of IDE state so I 
> think it's much better to protect this than later trying to debug such problems. (You 
> can't get rid of the flag without implementing mode switching that needs rewrite of 
> ISA and PCI emulation in QEMU so just get over it.)

Okay I'm getting confused now: I thought the original aim of this patchset was to 
allow a guest OS to switch VIA IDE to compatible mode by default? Then again 
whichever way you look at this, regardless of whether PCI_CLASS_PROG is set to 0x8a 
or 0x8f then QEMU's VIA IDE device is still advertising unsupported configurations to 
the guest OS.

0x8a means "I am currently in compatible mode but I can switch to native mode" so if 
this is the default then by definition the legacy IDE ioports *MUST* be accessible by 
the guest. Otherwise a guest OS will read PCI_CLASS_PROG and determine that the VIA 
IDE is in legacy mode, try to read the legacy IDE ioport and break which is what is 
happening now.

0x8f means "I am currently in native mode" but this is also a lie to the guest since 
whilst the BARs exist and can now be programmed thanks to your earlier patches, there 
is no PCI IRQ routing (i.e. no call to pci_set_irq()).

Fortunately with PCI_CLASS_PROG at 0x8a Linux will keep the VIA IDE in compatible 
mode and not attempt to switch to native mode: therefore if you keep this as-is and 
add the legacy IDE ioports back, that just leaves the problem with BAR4 (BMDMA). In 
effect your patch isn't setting compatible mode anymore, it is just disabling BMDMA.

Given that this is working around functionality currently missing in QEMU then you 
should rename the property "x-disable-bdma" to indicate it is experimental and add a 
comment explaining that this is a workaround for QEMU not being able to reset the 
BAR4 value to zero after reset. Hopefully one day the PCI bus will implement the 
Resettable interface at which point it should be possible to remove this hack.


ATB,

Mark.

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[BALATON Zoltan via]

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

On Tue, 29 Dec 2020, Mark Cave-Ayland wrote:
> On 28/12/2020 20:50, BALATON Zoltan via wrote:
>>>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>>>> index be09912b33..7d54d7e829 100644
>>>> --- a/hw/ide/via.c
>>>> +++ b/hw/ide/via.c
>>>> @@ -26,6 +26,7 @@
>>>>     #include "qemu/osdep.h"
>>>>   #include "hw/pci/pci.h"
>>>> +#include "hw/qdev-properties.h"
>>>>   #include "migration/vmstate.h"
>>>>   #include "qemu/module.h"
>>>>   #include "sysemu/dma.h"
>>>> @@ -185,12 +186,19 @@ static void via_ide_realize(PCIDevice *dev, Error 
>>>> **errp)
>>>>                             &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);
>>>> +    if (!(d->flags & BIT(PCI_IDE_LEGACY_MODE))) {
>>>> +        /* Missing BAR4 will make Linux driver fall back to legacy PIO 
>>>> mode */
>>>> +        bmdma_setup_bar(d);
>>>> +        pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, 
>>>> &d->bmdma_bar);
>>>> +    }
>>> 
>>> Since the default value of the legacy mode parameter is false, then this 
>>> means the device assumes native mode by default. Therefore PCI_CLASS_PROG 
>>> should be set to 0x8f unless legacy mode is being used, in which case it 
>>> should be 0x8a.
>> 
>> I think this casued problems before because if it's not set to 0x8a 
>> (legacy) at start then guests may assume it's already switched to native 
>> mode by firmware and won't program the BARs and it will not work. This way, 
>> even if it looks odd all guests I've tested work so I don't want to touch 
>> this, because I don't want to test all guests again.
>> 
>> Keep in mind we're not fully emulating this device so not every combination 
>> that may work on real hardware work in this model. We really either only 
>> emulate "half-native" mode (i.e. native mode with ISA IRQs) that's needed 
>> for pegasos2 guests or now again only emulate legacy mode if property is 
>> set for Linux on fuloong2e. (My original patch emulated half-native and 
>> native mode instead of legacy thinking that Linux on fuloong2e would 
>> adapt.) All other combinations, including switching between these two is 
>> expected to not work which is due to QEMU limitations as you've also 
>> discovered now. I think this is still an improvement over only emulating 
>> legacy mode before even if it does not yet fully model the chip and I've 
>> also cleaned up PCI IDE emulation during implementing this so that code can 
>> be shared between via-ide, sii3112 and CMD646 without much duplication.
>> 
>>>>       qdev_init_gpio_in(ds, via_ide_set_irq, 2);
>>>>       for (i = 0; i < 2; i++) {
>>>>           ide_bus_new(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
>>>> +        if (d->flags & BIT(PCI_IDE_LEGACY_MODE)) {
>>>> +            ide_init_ioport(&d->bus[i], NULL, i ? 0x170 : 0x1f0,
>>>> +                            i ? 0x376 : 0x3f6);
>>>> +        }
>>> 
>>> You could actually remove the if() here: PCI configuration always leaves a 
>>> gap at the lower end of IO address space to avoid clashes with legacy 
>>> ports. Therefore if a guest decides to switch to native mode and configure 
>>> the BAR, it will never clash with the default legacy IDE ports. This has 
>>> the advantage of minimising the parts protected by PCI_IDE_LEGACY_MODE 
>>> whilst also providing the legacy ports if someone can devise a method to 
>>> dynamically switch between compatible and native modes later.
>> 
>> I think leaving the legacy ports enabled is a bad idea for at least two 
>> reasons: 1) It may clash with other io ports on other machines, e.g. I'm 
>> not sure on PPC where firmware or OS does not expect to see legacy ISA 
>> ports won't map some io BAR of a PCI card there. 2) If this is left enabled 
>> there would be two ports poking the same registers so if that does not 
>> cause a problem by itself, writing to one accidentally (like when something 
>> is mapped over it) could cause corruption of IDE state so I think it's much 
>> better to protect this than later trying to debug such problems. (You can't 
>> get rid of the flag without implementing mode switching that needs rewrite 
>> of ISA and PCI emulation in QEMU so just get over it.)
>
> Okay I'm getting confused now: I thought the original aim of this patchset 
> was to allow a guest OS to switch VIA IDE to compatible mode by default? Then

No, switching is still not supported and as you also discovered it's not 
easy to implement in QEMU so aim is to have property to set if we want to 
emulate legacy mode only or (half) native mode only.

> again whichever way you look at this, regardless of whether PCI_CLASS_PROG is 
> set to 0x8a or 0x8f then QEMU's VIA IDE device is still advertising 
> unsupported configurations to the guest OS.

Yes, we're not fully emulating this device (see above) but advertise 
configurations that work WITH THE GUESTS! Pegasos2 guests will read 
PCI_CLASS_PROG and then switch to native mode (but still expecting 
interrupts on IRQ 14/15) so nothing will actually try to use legacy mode 
there but if it's not in that mode at the start it may not correctly 
program native mode. This may confuse other guests but I don't care as 
long as those we do care about work. Once I finish with all other 
outstanding issues I may come back and polish this model further but for 
now it looks unnecessary push back and this would be enough to continue.

> 0x8a means "I am currently in compatible mode but I can switch to native 
> mode" so if this is the default then by definition the legacy IDE ioports 
> *MUST* be accessible by the guest. Otherwise a guest OS will read 
> PCI_CLASS_PROG and determine that the VIA IDE is in legacy mode, try to read 
> the legacy IDE ioport and break which is what is happening now.

In theory. In practice guests don't really care about this flag or ports 
but have all kinds of assumptions and expectations how this is set up at 
the start or how it works on diffent boards. At least AmigaOS or MorphOS I 
think did not look at any of these just poked some regs and expected this 
to get to the state that's on real hardware. Which now works with these 
patches and also keeps fuloong2e work. What else do you want here?

> 0x8f means "I am currently in native mode" but this is also a lie to the 
> guest since whilst the BARs exist and can now be programmed thanks to your 
> earlier patches, there is no PCI IRQ routing (i.e. no call to pci_set_irq()).

Still this is what native mode means on pegasos2. It may be different on 
different boards this chip appears on but we don't emulate any of those in 
QEMU so it would be enough to address that when such a board is ever 
added. (I doubt that will happen so likely no need for more complete model 
at least for now.)

> Fortunately with PCI_CLASS_PROG at 0x8a Linux will keep the VIA IDE in 
> compatible mode and not attempt to switch to native mode: therefore if you 
> keep this as-is and add the legacy IDE ioports back, that just leaves the 
> problem with BAR4 (BMDMA). In effect your patch isn't setting compatible mode 
> anymore, it is just disabling BMDMA.

It's actually Guenter's patch so you're now bashing him not me...

> Given that this is working around functionality currently missing in QEMU 
> then you should rename the property "x-disable-bdma" to indicate it is 
> experimental and add a comment explaining that this is a workaround for QEMU 
> not being able to reset the BAR4 value to zero after reset. Hopefully one day 
> the PCI bus will implement the Resettable interface at which point it should 
> be possible to remove this hack.

Yes it's a hack and I did not claim it fully implements the device and I 
don't plan to do that because it would need more extensive clean ups in 
other parts of QEMU I'm not ready to take up. My goal is to get guests 
work with pegasos2 and maybe improve fuloong2e along the way (or at least 
not break it) but that's it. I did try to impmlement both modes first but 
gave up when saw it's not really possible with current ISA and PCI IDE 
code so the result is this compromise that's still achieves what I need.

Unless Philippe also thinks these changes are needed I don't really want 
to make any more changes to this. I think the commit message and these 
messages in list archives already describe this more than enough. But as 
I've said if you or someone else provides an alternative or make this 
change I'm OK with it as long as it still works with pegasos2. (I think 
Guenter's stance was the same with Linux and fuloong2e.)

(But I also think your time could be better spent than getting rid of this 
hack when there are much more hacks or missing functionalities to get rid 
of in the part you maintain.)

Regards,
BALATON Zoltan

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[Mark Cave-Ayland]

On 29/12/2020 12:01, BALATON Zoltan via wrote:

>> Fortunately with PCI_CLASS_PROG at 0x8a Linux will keep the VIA IDE in compatible 
>> mode and not attempt to switch to native mode: therefore if you keep this as-is and 
>> add the legacy IDE ioports back, that just leaves the problem with BAR4 (BMDMA). In 
>> effect your patch isn't setting compatible mode anymore, it is just disabling BMDMA.
> 
> It's actually Guenter's patch so you're now bashing him not me...

This is a deliberately misleading comment, and not a good introduction for someone 
external becoming involved with the project. Guenter's patch was a PoC demonstrating 
how to fix the fuloong2e machine, which is really appreciated since it clearly 
locates the problems to allow a fix to be applied upstream.

> (But I also think your time could be better spent than getting rid of this hack when 
> there are much more hacks or missing functionalities to get rid of in the part you 
> maintain.)

And comments like this are not appropriate for a technical mailing list either. I've 
done my best to review your patch in good faith (including reading related 
specifications and testing your pegasos2 model) and explain why it isn't reporting 
the correct information to the guest.

Phil - I hope you find that found my review comments useful and that they explain why 
the patchset is wrong by always claiming legacy IDE ioports exist but not providing 
them unless the new option is set (and indeed indicating some of the shortcomings of 
QEMU related to PCI BARs in this area that can be improved in future). As I feel 
comments in this thread have become directed at me personally, I am going to take a 
step back from this.


ATB,

Mark.

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[BALATON Zoltan via]

On Tue, 29 Dec 2020, Mark Cave-Ayland wrote:
> On 29/12/2020 12:01, BALATON Zoltan via wrote:
>>> Fortunately with PCI_CLASS_PROG at 0x8a Linux will keep the VIA IDE in 
>>> compatible mode and not attempt to switch to native mode: therefore if you 
>>> keep this as-is and add the legacy IDE ioports back, that just leaves the 
>>> problem with BAR4 (BMDMA). In effect your patch isn't setting compatible 
>>> mode anymore, it is just disabling BMDMA.
>> 
>> It's actually Guenter's patch so you're now bashing him not me...
>
> This is a deliberately misleading comment, and not a good introduction for

It's not deliberately misleading just stating a fact that you deliberately 
misinterpret. Guenter has contributed before (he also fixed up my sam460ex 
emulation) and his contributions are certainly appreciated. I'm sorry that 
he got unintentinally invoved in this disagreement between us two but I 
think he can ignore all this without a problem. I did not mean to drag him 
into it, just pointed out what you're doing and I think he got that.

> someone external becoming involved with the project. Guenter's patch was a 
> PoC demonstrating how to fix the fuloong2e machine, which is really 
> appreciated since it clearly locates the problems to allow a fix to be 
> applied upstream.
>
>> (But I also think your time could be better spent than getting rid of this 
>> hack when there are much more hacks or missing functionalities to get rid 
>> of in the part you maintain.)
>
> And comments like this are not appropriate for a technical mailing list 
> either. I've done my best to review your patch in good faith (including

Don't misunderstand this again, I did not mean to say that you made 
mistakes (although everyone does so that's not a problem) but what I meant 
is that there are a lot of things even in your areas that could be 
improved and that's time much better spent than discussing this patch 
endlessly on phylosophical basis when it's unlikely to get better.

> reading related specifications and testing your pegasos2 model) and explain 
> why it isn't reporting the correct information to the guest.

Yes I agree this should be brought to off list and clear up this between 
us I just don't have time for it now but I'll write to you later. As for 
your comments, we've been through all this in March and I get the 
impression that whatever I submit is criticised by you all the time so I 
really wonder if it's against me personally or you just getting old and 
grumpy :-) Don't take this as personal, no insult is meant just want to 
know if I did something that made you change your mind about my 
contributions or you do this to everything submitted to QEMU, because 
that's slowly becomes a burden discouriging me to contribute anything. I 
already gave up contributing to OpenBIOS but won't give up with QEMU, but 
I'm a bit tired having to fight for every little change to get past you 
(even in areas you're not maintaining like this one).

You can answer in private to this, I think others will be greateful to be 
left out of the discussion.

> Phil - I hope you find that found my review comments useful and that they 
> explain why the patchset is wrong by always claiming legacy IDE ioports exist 
> but not providing them unless the new option is set (and indeed indicating 
> some of the shortcomings of QEMU related to PCI BARs in this area that can be 
> improved in future). As I feel comments in this thread have become directed 
> at me personally, I am going to take a step back from this.

I'm sorry if you feel my replies got personal but I also feel your 
comments getting at me and you seemed to critique something that was 
addmittedly not perfect but working and demanded perfection where it's not 
feasible (without way more work that you can expect from unpayed 
contributors) and calling my patches wrong for that reason. I don't mind 
if you add comments, warnnings or change the commit message to say "this 
patch is all wrong but fixes Linux on fuloong2e and makes guests work with 
pegasos2 within the constraints of QEMU" as long as it gets in until a 
better fix may be available sometimes in the future. But:

- this patch is not mine now
- I did change what you asked in the first review but then you came back with this
- all this has been discussed to death and everyone but you seemed to accept it

I'll try to refrain from answering any more about this and let's continue 
off list if you want. I'd really want to avoid further confrontation but I 
happen to contribute to parts you also have an interest in so it's hard to 
avid each other so it would be better to get to some acceptable terms that 
allow me to contribute without upsetting you too much.

Regards,
BALATON Zoltan

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[Mark Cave-Ayland]

On 28/12/2020 20:50, BALATON Zoltan via wrote:

> I think leaving the legacy ports enabled is a bad idea for at least two reasons: 1) 
> It may clash with other io ports on other machines, e.g. I'm not sure on PPC where 
> firmware or OS does not expect to see legacy ISA ports won't map some io BAR of a PCI 
> card there. 2) If this is left enabled there would be two ports poking the same 
> registers so if that does not cause a problem by itself, writing to one accidentally 
> (like when something is mapped over it) could cause corruption of IDE state so I 
> think it's much better to protect this than later trying to debug such problems.

Legacy ioports originate in the x86 world, however all the PCI bus enumeration code 
I've seen reserves the lower part of the IO address space to prevent such problems 
with e.g. a BIOS starting up in legacy mode.


ATB,

Mark.

Re: [PATCH v2 2/2] via-ide: Fix fuloong2e support

[BALATON Zoltan via]

On Tue, 29 Dec 2020, Mark Cave-Ayland wrote:
> On 28/12/2020 20:50, BALATON Zoltan via wrote:
>> I think leaving the legacy ports enabled is a bad idea for at least two 
>> reasons: 1) It may clash with other io ports on other machines, e.g. I'm 
>> not sure on PPC where firmware or OS does not expect to see legacy ISA 
>> ports won't map some io BAR of a PCI card there. 2) If this is left enabled 
>> there would be two ports poking the same registers so if that does not 
>> cause a problem by itself, writing to one accidentally (like when something 
>> is mapped over it) could cause corruption of IDE state so I think it's much 
>> better to protect this than later trying to debug such problems.
>
> Legacy ioports originate in the x86 world, however all the PCI bus 
> enumeration code I've seen reserves the lower part of the IO address space to 
> prevent such problems with e.g. a BIOS starting up in legacy mode.

I don't remember the details and won't test it again but PPC has neither 
BIOS nor legacy io ports (or io ports for that matter, all that is memory 
mapped). If you want go back to the email thread in March where I've 
described in detail how I ended up with these and what are the assumptions 
of guests I've tested and tried to satisfy with this.

Stop trying to compare it with hardware and look at it in a way that we 
want to make a device model that works with the guests we want to run. In 
that frame this works and probably the simplest way. Unless you fully want 
to implement all the quirks of the chip and take up all the clean up in 
QEMU needed for that (possibly risking breaking a lot of other boards 
along the way) this won't match hardware 100%.

Regards,
BALATON Zoltan

[PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode

[BALATON Zoltan via]

We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for further flags if needed in
the future. This patch changes the "secondary" field to "flags" and
change CMD646 and its users accordingly and define a new flag for
forcing legacy mode that will be used by via-ide for now.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Fixed typo in commit message

 hw/ide/cmd646.c      | 4 ++--
 hw/sparc64/sun4u.c   | 2 +-
 include/hw/ide/pci.h | 7 ++++++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c254631485..7a96016116 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     pci_conf[PCI_CLASS_PROG] = 0x8f;
 
     pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
-    if (d->secondary) {
+    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
         /* XXX: if not enabled, really disable the seconday IDE controller */
         pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
     }
@@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
 }
 
 static Property cmd646_ide_properties[] = {
-    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
+    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index 0fa13a7330..c46baa9f48 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
     }
 
     pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
-    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
+    qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
     pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
     pci_ide_create_devs(pci_dev);
 
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..75d1a32f6d 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -42,6 +42,11 @@ typedef struct BMDMAState {
 #define TYPE_PCI_IDE "pci-ide"
 OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
 
+enum {
+    PCI_IDE_SECONDARY, /* used only for cmd646 */
+    PCI_IDE_LEGACY_MODE
+};
+
 struct PCIIDEState {
     /*< private >*/
     PCIDevice parent_obj;
@@ -49,7 +54,7 @@ struct PCIIDEState {
 
     IDEBus bus[2];
     BMDMAState bmdma[2];
-    uint32_t secondary; /* used only for cmd646 */
+    uint32_t flags;
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];
-- 
2.21.3

Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode

[Mark Cave-Ayland]

On 27/12/2020 22:13, BALATON Zoltan wrote:

> We'll need a flag for implementing some device specific behaviour in
> via-ide but we already have a currently CMD646 specific field that can
> be repurposed for this and leave room for further flags if needed in
> the future. This patch changes the "secondary" field to "flags" and
> change CMD646 and its users accordingly and define a new flag for
> forcing legacy mode that will be used by via-ide for now.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Fixed typo in commit message
> 
>   hw/ide/cmd646.c      | 4 ++--
>   hw/sparc64/sun4u.c   | 2 +-
>   include/hw/ide/pci.h | 7 ++++++-
>   3 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index c254631485..7a96016116 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
>       pci_conf[PCI_CLASS_PROG] = 0x8f;
>   
>       pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0

Doesn't the existing comment above cause checkpatch to fail?

> -    if (d->secondary) {
> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>           /* XXX: if not enabled, really disable the seconday IDE controller */
>           pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>       }
> @@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>   }
>   
>   static Property cmd646_ide_properties[] = {
> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, false),
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 0fa13a7330..c46baa9f48 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion *address_space_mem,
>       }
>   
>       pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
> +    qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
>       pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
>       pci_ide_create_devs(pci_dev);
>   
> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
> index d8384e1c42..75d1a32f6d 100644
> --- a/include/hw/ide/pci.h
> +++ b/include/hw/ide/pci.h
> @@ -42,6 +42,11 @@ typedef struct BMDMAState {
>   #define TYPE_PCI_IDE "pci-ide"
>   OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
>   
> +enum {
> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
> +    PCI_IDE_LEGACY_MODE
> +};
> +
>   struct PCIIDEState {
>       /*< private >*/
>       PCIDevice parent_obj;
> @@ -49,7 +54,7 @@ struct PCIIDEState {
>   
>       IDEBus bus[2];
>       BMDMAState bmdma[2];
> -    uint32_t secondary; /* used only for cmd646 */
> +    uint32_t flags;
>       MemoryRegion bmdma_bar;
>       MemoryRegion cmd_bar[2];
>       MemoryRegion data_bar[2];

Other than that I think this looks okay.


ATB,

Mark.

Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode

[BALATON Zoltan via]

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

On Mon, 28 Dec 2020, Mark Cave-Ayland wrote:
> On 27/12/2020 22:13, BALATON Zoltan wrote:
>
>> We'll need a flag for implementing some device specific behaviour in
>> via-ide but we already have a currently CMD646 specific field that can
>> be repurposed for this and leave room for further flags if needed in
>> the future. This patch changes the "secondary" field to "flags" and
>> change CMD646 and its users accordingly and define a new flag for
>> forcing legacy mode that will be used by via-ide for now.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Fixed typo in commit message
>>
>>   hw/ide/cmd646.c      | 4 ++--
>>   hw/sparc64/sun4u.c   | 2 +-
>>   include/hw/ide/pci.h | 7 ++++++-
>>   3 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index c254631485..7a96016116 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, 
>> Error **errp)
>>       pci_conf[PCI_CLASS_PROG] = 0x8f;
>>         pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
>
> Doesn't the existing comment above cause checkpatch to fail?

It didn't (nether when I ran it nor in patchew) but I can fix it if I send 
a new version.

Regards,
BALATON Zoltan

>> -    if (d->secondary) {
>> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>>           /* XXX: if not enabled, really disable the seconday IDE 
>> controller */
>>           pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */
>>       }
>> @@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>   }
>>     static Property cmd646_ide_properties[] = {
>> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
>> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags, PCI_IDE_SECONDARY, 
>> false),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index 0fa13a7330..c46baa9f48 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -674,7 +674,7 @@ static void sun4uv_init(MemoryRegion 
>> *address_space_mem,
>>       }
>>         pci_dev = pci_new(PCI_DEVFN(3, 0), "cmd646-ide");
>> -    qdev_prop_set_uint32(&pci_dev->qdev, "secondary", 1);
>> +    qdev_prop_set_bit(&pci_dev->qdev, "secondary", true);
>>       pci_realize_and_unref(pci_dev, pci_busA, &error_fatal);
>>       pci_ide_create_devs(pci_dev);
>>   diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
>> index d8384e1c42..75d1a32f6d 100644
>> --- a/include/hw/ide/pci.h
>> +++ b/include/hw/ide/pci.h
>> @@ -42,6 +42,11 @@ typedef struct BMDMAState {
>>   #define TYPE_PCI_IDE "pci-ide"
>>   OBJECT_DECLARE_SIMPLE_TYPE(PCIIDEState, PCI_IDE)
>>   +enum {
>> +    PCI_IDE_SECONDARY, /* used only for cmd646 */
>> +    PCI_IDE_LEGACY_MODE
>> +};
>> +
>>   struct PCIIDEState {
>>       /*< private >*/
>>       PCIDevice parent_obj;
>> @@ -49,7 +54,7 @@ struct PCIIDEState {
>>         IDEBus bus[2];
>>       BMDMAState bmdma[2];
>> -    uint32_t secondary; /* used only for cmd646 */
>> +    uint32_t flags;
>>       MemoryRegion bmdma_bar;
>>       MemoryRegion cmd_bar[2];
>>       MemoryRegion data_bar[2];
>
> Other than that I think this looks okay.
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH v2 1/2] ide: Make room for flags in PCIIDEState and add one for legacy mode

[Philippe Mathieu-Daudé]

On 12/28/20 8:30 PM, Mark Cave-Ayland wrote:
> On 27/12/2020 22:13, BALATON Zoltan wrote:
> 
>> We'll need a flag for implementing some device specific behaviour in
>> via-ide but we already have a currently CMD646 specific field that can
>> be repurposed for this and leave room for further flags if needed in
>> the future. This patch changes the "secondary" field to "flags" and
>> change CMD646 and its users accordingly and define a new flag for
>> forcing legacy mode that will be used by via-ide for now.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Fixed typo in commit message
>>
>>   hw/ide/cmd646.c      | 4 ++--
>>   hw/sparc64/sun4u.c   | 2 +-
>>   include/hw/ide/pci.h | 7 ++++++-
>>   3 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index c254631485..7a96016116 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -256,7 +256,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev,
>> Error **errp)
>>       pci_conf[PCI_CLASS_PROG] = 0x8f;
>>         pci_conf[CNTRL] = CNTRL_EN_CH0; // enable IDE0
> 
> Doesn't the existing comment above cause checkpatch to fail?

The comment is old and Balaton didn't modified it. Usually I'd prefer
to address style change in a separate commit, ...

> 
>> -    if (d->secondary) {
>> +    if (d->flags & BIT(PCI_IDE_SECONDARY)) {
>>           /* XXX: if not enabled, really disable the seconday IDE
>> controller */
>>           pci_conf[CNTRL] |= CNTRL_EN_CH1; /* enable IDE1 */

... but since a similar comment is added here, it might be acceptable
to fix the style of the former one here too. I noted Balaton already
addressed your comment in a v3.

>>       }
>> @@ -314,7 +314,7 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
>>   }
>>     static Property cmd646_ide_properties[] = {
>> -    DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
>> +    DEFINE_PROP_BIT("secondary", PCIIDEState, flags,
>> PCI_IDE_SECONDARY, false),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };