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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.