Re: [PATCH v2 19/43] hw/isa/piix3: Allow board to provide PCI interrupt routes

[Bernhard Beschow]

Hi Phil,

Am 25. Oktober 2022 23:34:15 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>On 22/10/22 17:04, Bernhard Beschow wrote:
>> PIIX3 initializes the PIRQx route control registers to the default
>> values as described in the 82371AB PCI-TO-ISA/IDE XCELERATOR (PIIX4)
>> April 1997 manual. PIIX4, however, initializes the routes according to
>> the Malta™ User’s Manual, ch 6.6, which are IRQs 10 and 11. In order to
>> allow the reset methods to be consolidated, allow board code to specify
>> the routes.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/isa/piix3.c                | 12 ++++++++----
>>   include/hw/southbridge/piix.h |  1 +
>>   2 files changed, 9 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>> index aa32f43e4a..c6a8f1f27d 100644
>> --- a/hw/isa/piix3.c
>> +++ b/hw/isa/piix3.c
>> @@ -168,10 +168,10 @@ static void piix3_reset(DeviceState *dev)
>>       pci_conf[0x4c] = 0x4d;
>>       pci_conf[0x4e] = 0x03;
>>       pci_conf[0x4f] = 0x00;
>> -    pci_conf[0x60] = 0x80;
>> -    pci_conf[0x61] = 0x80;
>> -    pci_conf[0x62] = 0x80;
>> -    pci_conf[0x63] = 0x80;
>
>These values are the correct reset ones however (also for PIIX4).
>
>The problem is the Malta machine abuse of the PIIX4 model. IOW
>this doesn't seem the correct approach, we should fix how Malta
>use the PIIX4 (in the emulated tiny boot loader). I'll try to
>write smth before reviewing the rest of this series, because
>it might simplify your refactor.

Indeed my first approach for the refactor was to implement MachineClass::reset 
for Malta where the Malta-specific values would be set. I could redo that if 
you want. Just let me know.

Best regards,
Bernhard
>
>> +    pci_conf[PIIX_PIRQCA] = d->pci_irq_reset_mappings[0];
>> +    pci_conf[PIIX_PIRQCB] = d->pci_irq_reset_mappings[1];
>> +    pci_conf[PIIX_PIRQCC] = d->pci_irq_reset_mappings[2];
>> +    pci_conf[PIIX_PIRQCD] = d->pci_irq_reset_mappings[3];
>>       pci_conf[0x69] = 0x02;
>>       pci_conf[0x70] = 0x80;
>>       pci_conf[0x76] = 0x0c;
>> @@ -383,6 +383,10 @@ static void pci_piix3_init(Object *obj)
>>     static Property pci_piix3_props[] = {
>>       DEFINE_PROP_UINT32("smb_io_base", PIIX3State, smb_io_base, 0),
>> +    DEFINE_PROP_UINT8("pirqa", PIIX3State, pci_irq_reset_mappings[0], 0x80),
>> +    DEFINE_PROP_UINT8("pirqb", PIIX3State, pci_irq_reset_mappings[1], 0x80),
>> +    DEFINE_PROP_UINT8("pirqc", PIIX3State, pci_irq_reset_mappings[2], 0x80),
>> +    DEFINE_PROP_UINT8("pirqd", PIIX3State, pci_irq_reset_mappings[3], 0x80),
>>       DEFINE_PROP_BOOL("has-acpi", PIIX3State, has_acpi, true),
>>       DEFINE_PROP_BOOL("has-usb", PIIX3State, has_usb, true),
>>       DEFINE_PROP_BOOL("smm-enabled", PIIX3State, smm_enabled, false),
>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>> index 1f22eb1444..df3e0084c5 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -54,6 +54,7 @@ struct PIIXState {
>>         /* This member isn't used. Just for save/load compatibility */
>>       int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
>> +    uint8_t pci_irq_reset_mappings[PIIX_NUM_PIRQS];
>>         ISAPICState pic;
>>       RTCState rtc;
>