Re: [PATCH v2 2/5] ipmi: add fwinfo to pci ipmi devices

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

On 1/4/25 15:27, Nicholas Piggin wrote:
> On Tue Apr 1, 2025 at 9:57 PM AEST, Philippe Mathieu-Daudé wrote:
>> Hi Nick,
>>
>> On 1/4/25 13:44, Nicholas Piggin wrote:
>>> This requires some adjustments to callers to avoid possible behaviour
>>> changes for PCI devices.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    include/hw/ipmi/ipmi.h     |  5 +++++
>>>    hw/acpi/ipmi.c             |  2 +-
>>>    hw/ipmi/isa_ipmi_bt.c      |  1 +
>>>    hw/ipmi/isa_ipmi_kcs.c     |  1 +
>>>    hw/ipmi/pci_ipmi_bt.c      | 12 ++++++++++++
>>>    hw/ipmi/pci_ipmi_kcs.c     | 11 +++++++++++
>>>    hw/smbios/smbios_type_38.c |  6 +++++-
>>>    7 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
>>> index 77a7213ed93..71c4efac8cd 100644
>>> --- a/include/hw/ipmi/ipmi.h
>>> +++ b/include/hw/ipmi/ipmi.h
>>> @@ -90,6 +90,11 @@ typedef struct IPMIFwInfo {
>>>        } memspace;
>>>    
>>>        int interrupt_number;
>>> +    enum {
>>> +        IPMI_NO_IRQ = 0,
>>> +        IPMI_ISA_IRQ,
>>> +        IPMI_PCI_IRQ,
>>> +    } irq;
>>>        enum {
>>>            IPMI_LEVEL_IRQ,
>>>            IPMI_EDGE_IRQ
>>> diff --git a/hw/acpi/ipmi.c b/hw/acpi/ipmi.c
>>> index a20e57d465c..c81cbd2f158 100644
>>> --- a/hw/acpi/ipmi.c
>>> +++ b/hw/acpi/ipmi.c
>>> @@ -55,7 +55,7 @@ static Aml *aml_ipmi_crs(IPMIFwInfo *info)
>>>            abort();
>>>        }
>>>    
>>> -    if (info->interrupt_number) {
>>> +    if (info->irq == IPMI_ISA_IRQ && info->interrupt_number) {
>>>            aml_append(crs, aml_irq_no_flags(info->interrupt_number));
>>>        }
>>>    
>>> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
>>> index a1b66d5ee82..b5556436b82 100644
>>> --- a/hw/ipmi/isa_ipmi_bt.c
>>> +++ b/hw/ipmi/isa_ipmi_bt.c
>>> @@ -49,6 +49,7 @@ static void isa_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>>>        ISAIPMIBTDevice *iib = ISA_IPMI_BT(ii);
>>>    
>>>        ipmi_bt_get_fwinfo(&iib->bt, info);
>>> +    info->irq = IPMI_ISA_IRQ;
>>>        info->interrupt_number = iib->isairq;
>>>        info->i2c_slave_address = iib->bt.bmc->slave_addr;
>>>        info->uuid = iib->uuid;
>>> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
>>> index d9ebdd5371f..326115f51bb 100644
>>> --- a/hw/ipmi/isa_ipmi_kcs.c
>>> +++ b/hw/ipmi/isa_ipmi_kcs.c
>>> @@ -49,6 +49,7 @@ static void isa_ipmi_kcs_get_fwinfo(IPMIInterface *ii, IPMIFwInfo *info)
>>>        ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(ii);
>>>    
>>>        ipmi_kcs_get_fwinfo(&iik->kcs, info);
>>> +    info->irq = IPMI_ISA_IRQ;
>>>        info->interrupt_number = iik->isairq;
>>>        info->uuid = iik->uuid;
>>>    }
>>> diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
>>> index a3b742d22c9..33ff7190ee8 100644
>>> --- a/hw/ipmi/pci_ipmi_bt.c
>>> +++ b/hw/ipmi/pci_ipmi_bt.c
>>> @@ -38,6 +38,17 @@ struct PCIIPMIBTDevice {
>>>        uint32_t uuid;
>>>    };
>>>    
>>> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>>> +{
>>> +    PCIIPMIBTDevice *pib = PCI_IPMI_BT(ii);
>>> +
>>> +    ipmi_bt_get_fwinfo(&pib->bt, info);
>>> +    info->irq = IPMI_PCI_IRQ;
>>> +    info->interrupt_number = pci_intx(&pib->dev);
>>> +    info->i2c_slave_address = pib->bt.bmc->slave_addr;
>>> +    info->uuid = pib->uuid;
>>> +}
>>> +
>>>    static void pci_ipmi_raise_irq(IPMIBT *ib)
>>>    {
>>>        PCIIPMIBTDevice *pib = ib->opaque;
>>> @@ -125,6 +136,7 @@ static void pci_ipmi_bt_class_init(ObjectClass *oc, void *data)
>>>    
>>>        iic->get_backend_data = pci_ipmi_bt_get_backend_data;
>>>        ipmi_bt_class_init(iic);
>>> +    iic->get_fwinfo = pci_ipmi_bt_get_fwinfo;
>>>    }
>>>    
>>>    static const TypeInfo pci_ipmi_bt_info = {
>>> diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
>>> index 05ba97ec58f..6673b2088ef 100644
>>> --- a/hw/ipmi/pci_ipmi_kcs.c
>>> +++ b/hw/ipmi/pci_ipmi_kcs.c
>>> @@ -38,6 +38,16 @@ struct PCIIPMIKCSDevice {
>>>        uint32_t uuid;
>>>    };
>>>    
>>> +static void pci_ipmi_bt_get_fwinfo(struct IPMIInterface *ii, IPMIFwInfo *info)
>>> +{
>>> +    PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(ii);
>>> +
>>> +    ipmi_kcs_get_fwinfo(&pik->kcs, info);
>>> +    info->irq = IPMI_PCI_IRQ;
>>> +    info->interrupt_number = pci_intx(&pik->dev);
>>> +    info->uuid = pik->uuid;
>>> +}
>>> +
>>>    static void pci_ipmi_raise_irq(IPMIKCS *ik)
>>>    {
>>>        PCIIPMIKCSDevice *pik = ik->opaque;
>>> @@ -125,6 +135,7 @@ static void pci_ipmi_kcs_class_init(ObjectClass *oc, void *data)
>>>    
>>>        iic->get_backend_data = pci_ipmi_kcs_get_backend_data;
>>>        ipmi_kcs_class_init(iic);
>>> +    iic->get_fwinfo = pci_ipmi_kcs_get_fwinfo;
>>>    }
>>>    
>>>    static const TypeInfo pci_ipmi_kcs_info = {
>>> diff --git a/hw/smbios/smbios_type_38.c b/hw/smbios/smbios_type_38.c
>>> index 168b886647d..2823929c258 100644
>>> --- a/hw/smbios/smbios_type_38.c
>>> +++ b/hw/smbios/smbios_type_38.c
>>> @@ -72,7 +72,11 @@ static void smbios_build_one_type_38(IPMIFwInfo *info)
>>>                         " SMBIOS, ignoring this entry.", info->register_spacing);
>>>            return;
>>>        }
>>> -    t->interrupt_number = info->interrupt_number;
>>> +    if (info->irq == IPMI_ISA_IRQ) {
>>> +        t->interrupt_number = info->interrupt_number;
>>> +    } else {
>>> +        t->interrupt_number = 0;
>>
>> Can you explain why use 0 for PCI?
> 
> To avoid changes to other callers after this patch. Previously PCI
> devices would leave interrupt_number as 0, not sure if such devices
> are relevant here. If there is a better approach I would take it.

OK, this is what I thought but I felt I could have missed something.
Thanks!