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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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");
>>     }