Re: [PATCH 09/14] hw/southbridge/ich9: Add a AHCI function

[Philippe Mathieu-Daudé]

On 19/2/24 19:31, BALATON Zoltan wrote:
> On Mon, 19 Feb 2024, Philippe Mathieu-Daudé wrote:
> > Instantiate TYPE_ICH9_AHCI in TYPE_ICH9_SOUTHBRIDGE.
> >
> > Since the PC machines can disable SATA (see the
> > PC_MACHINE_SATA dynamic property), add the 'sata-enabled'
> > property to disable it.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > MAINTAINERS                   |  2 ++
> > include/hw/southbridge/ich9.h |  4 ----
> > hw/i386/pc_q35.c              | 25 ++++---------------------
> > hw/southbridge/ich9.c         | 35 +++++++++++++++++++++++++++++++++++
> > hw/i386/Kconfig               |  1 -
> > hw/southbridge/Kconfig        |  1 +
> > 6 files changed, 42 insertions(+), 26 deletions(-)


> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index 2f15af540f..060358d449 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -61,9 +61,6 @@
> > #include "hw/acpi/acpi.h"
> > #include "target/i386/cpu.h"
> >
> > -/* ICH9 AHCI has 6 ports */
> > -#define MAX_SATA_PORTS     6
> > -
> > struct ehci_companions {
> >     const char *name;
> >     int func;
> > @@ -129,7 +126,7 @@ static void pc_q35_init(MachineState *machine)
> >     PCIDevice *lpc;
> >     Object *lpc_obj;
> >     DeviceState *lpc_dev;
> > -    BusState *idebus[MAX_SATA_PORTS];
> > +    BusState *idebus[2] = { };

[*]

> >     ISADevice *rtc_state;
> >     MemoryRegion *system_memory = get_system_memory();
> >     MemoryRegion *system_io = get_system_io();
> > @@ -138,7 +135,6 @@ static void pc_q35_init(MachineState *machine)
> >     ISABus *isa_bus;
> >     int i;
> >     ram_addr_t lowmem;
> > -    DriveInfo *hd[MAX_SATA_PORTS];
> >     MachineClass *mc = MACHINE_GET_CLASS(machine);
> >     bool acpi_pcihp;
> >     bool keep_pci_slot_hpc;
> > @@ -239,6 +235,7 @@ static void pc_q35_init(MachineState *machine)
> >     object_property_set_link(OBJECT(ich9), "mch-pcie-bus",
> >                              OBJECT(host_bus), &error_abort);
> >     qdev_prop_set_bit(ich9, "d2p-enabled", false);
> > +    qdev_prop_set_bit(ich9, "sata-enabled", pcms->sata_enabled);
> >     qdev_realize_and_unref(ich9, NULL, &error_fatal);
> >
> >     /* irq lines */
> > @@ -302,22 +299,8 @@ static void pc_q35_init(MachineState *machine)
> >                          0xff0104);
> >
> >     if (pcms->sata_enabled) {
>
> Shouldn't this condition be inverted if you only leave the else leg?

idebus[] is NULL-initialized in [*] so we can remove the else
ladder.

> Regards,.
> BALATON Zoltan
>
> > -        PCIDevice *pdev;
> > -        AHCIPCIState *ich9;
> > -
> > -        /* ahci and SATA device, for q35 1 ahci controller is 
> > built-in */
> > -        pdev = pci_create_simple_multifunction(host_bus,
> > -                                               PCI_DEVFN(ICH9_SATA1_DEV,
> > -                                                         
> > ICH9_SATA1_FUNC),
> > -                                               "ich9-ahci");
> > -        ich9 = ICH9_AHCI(pdev);
> > -        idebus[0] = qdev_get_child_bus(DEVICE(pdev), "ide.0");
> > -        idebus[1] = qdev_get_child_bus(DEVICE(pdev), "ide.1");
> > -        g_assert(MAX_SATA_PORTS == ich9->ahci.ports);
> > -        ide_drive_get(hd, ich9->ahci.ports);
> > -        ahci_ide_create_devs(&ich9->ahci, hd);
> > -    } else {
> > -        idebus[0] = idebus[1] = NULL;
> > +        idebus[0] = qdev_get_child_bus(ich9, "ide.0");
> > +        idebus[1] = qdev_get_child_bus(ich9, "ide.1");
> >     }