[Skiboot] [PATCH skiboot v2] opal: Deprecate reading the PHB status
Alexey Kardashevskiy
aik at ozlabs.ru
Tue Jan 29 13:29:55 AEDT 2019
On 11/01/2019 15:55, Andrew Donnellan wrote:
> On 11/1/19 3:03 pm, Alexey Kardashevskiy wrote:
>> The OPAL_PCI_EEH_FREEZE_STATUS call takes a bunch of parameters, one of
>> them is @phb_status. It is defined as __be64* and always NULL in
>> the current Linux upstream but if anyone ever decides to read that
>> status,
>> then the PHB3's handler will assume it is struct OpalIoPhb3ErrorData*
>> (which is a lot bigger than 8 bytes) and zero it causing the stack
>> corruption; p7ioc-phb has the same issue.
>>
>> This removes @phb_status from all eeh_freeze_status() hooks and moves
>> the error message from PHB4 to the affected OPAL handlers.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik at ozlabs.ru>
>
> LGTM
>
> Reviewed-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
Ping?
>
>> ---
>> Changes:
>> v2:
>> * instead of fixing just phb3, this removes phb_status from everywhere
>>
>> ---
>> include/npu2.h | 3 +--
>> include/pci.h | 3 +--
>> core/pci-opal.c | 14 ++++++++++++--
>> core/pci.c | 2 +-
>> hw/npu.c | 3 +--
>> hw/npu2.c | 3 +--
>> hw/p7ioc-phb.c | 11 +++--------
>> hw/phb3.c | 10 ++--------
>> hw/phb4.c | 9 ++-------
>> 9 files changed, 24 insertions(+), 34 deletions(-)
>>
>> diff --git a/include/npu2.h b/include/npu2.h
>> index 1de963d..dc5d48b 100644
>> --- a/include/npu2.h
>> +++ b/include/npu2.h
>> @@ -231,6 +231,5 @@ int64_t npu2_freeze_status(struct phb *phb __unused,
>> uint64_t pe_number __unused,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type __unused,
>> - uint16_t *severity __unused,
>> - uint64_t *phb_status __unused);
>> + uint16_t *severity __unused);
>> #endif /* __NPU2_H */
>> diff --git a/include/pci.h b/include/pci.h
>> index 0516561..b49641c 100644
>> --- a/include/pci.h
>> +++ b/include/pci.h
>> @@ -260,8 +260,7 @@ struct phb_ops {
>> int64_t (*eeh_freeze_status)(struct phb *phb, uint64_t pe_number,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type,
>> - uint16_t *severity,
>> - uint64_t *phb_status);
>> + uint16_t *severity);
>> int64_t (*eeh_freeze_clear)(struct phb *phb, uint64_t pe_number,
>> uint64_t eeh_action_token);
>> int64_t (*eeh_freeze_set)(struct phb *phb, uint64_t pe_number,
>> diff --git a/core/pci-opal.c b/core/pci-opal.c
>> index a4d6eee..d7abb15 100644
>> --- a/core/pci-opal.c
>> +++ b/core/pci-opal.c
>> @@ -111,8 +111,13 @@ static int64_t
>> opal_pci_eeh_freeze_status(uint64_t phb_id, uint64_t pe_number,
>> if (!phb->ops->eeh_freeze_status)
>> return OPAL_UNSUPPORTED;
>> phb_lock(phb);
>> +
>> + if (phb_status)
>> + prlog(PR_ERR, "PHB#%04llx: %s: deprecated PHB status\n",
>> + phb_id, __func__);
>> +
>> rc = phb->ops->eeh_freeze_status(phb, pe_number, freeze_state,
>> - pci_error_type, NULL, phb_status);
>> + pci_error_type, NULL);
>> phb_unlock(phb);
>> return rc;
>> @@ -961,8 +966,13 @@ static int64_t
>> opal_pci_eeh_freeze_status2(uint64_t phb_id, uint64_t pe_number,
>> if (!phb->ops->eeh_freeze_status)
>> return OPAL_UNSUPPORTED;
>> phb_lock(phb);
>> +
>> + if (phb_status)
>> + prlog(PR_ERR, "PHB#%04llx: %s: deprecated PHB status\n",
>> + phb_id, __func__);
>> +
>> rc = phb->ops->eeh_freeze_status(phb, pe_number, freeze_state,
>> - pci_error_type, severity, phb_status);
>> + pci_error_type, severity);
>> phb_unlock(phb);
>> return rc;
>> diff --git a/core/pci.c b/core/pci.c
>> index 14654d4..454b501 100644
>> --- a/core/pci.c
>> +++ b/core/pci.c
>> @@ -337,7 +337,7 @@ static void pci_check_clear_freeze(struct phb *phb)
>> /* Retrieve the frozen state */
>> rc = phb->ops->eeh_freeze_status(phb, pe_number, &freeze_state,
>> - &pci_error_type, &sev, NULL);
>> + &pci_error_type, &sev);
>> if (rc)
>> return;
>> if (freeze_state == OPAL_EEH_STOPPED_NOT_FROZEN)
>> diff --git a/hw/npu.c b/hw/npu.c
>> index 08eee61..f505ac7 100644
>> --- a/hw/npu.c
>> +++ b/hw/npu.c
>> @@ -882,8 +882,7 @@ static int64_t npu_freeze_status(struct phb *phb,
>> uint64_t pe_number __unused,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type __unused,
>> - uint16_t *severity __unused,
>> - uint64_t *phb_status __unused)
>> + uint16_t *severity __unused)
>> {
>> /* FIXME: When it's called by skiboot PCI config accessor,
>> * the PE number is fixed to 0, which is incorrect. We need
>> diff --git a/hw/npu2.c b/hw/npu2.c
>> index 4e75eeb..ea465f1 100644
>> --- a/hw/npu2.c
>> +++ b/hw/npu2.c
>> @@ -1319,8 +1319,7 @@ int64_t npu2_freeze_status(struct phb *phb
>> __unused,
>> uint64_t pe_number __unused,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type,
>> - uint16_t *severity,
>> - uint64_t *phb_status __unused)
>> + uint16_t *severity)
>> {
>> /*
>> * FIXME: When it's called by skiboot PCI config accessor,
>> diff --git a/hw/p7ioc-phb.c b/hw/p7ioc-phb.c
>> index e8730f4..f09e30c 100644
>> --- a/hw/p7ioc-phb.c
>> +++ b/hw/p7ioc-phb.c
>> @@ -284,8 +284,7 @@ static void p7ioc_eeh_read_phb_status(struct
>> p7ioc_phb *p,
>> static int64_t p7ioc_eeh_freeze_status(struct phb *phb, uint64_t
>> pe_number,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type,
>> - uint16_t *severity,
>> - uint64_t *phb_status)
>> + uint16_t *severity)
>> {
>> struct p7ioc_phb *p = phb_to_p7ioc_phb(phb);
>> uint64_t peev_bit = PPC_BIT(pe_number & 0x3f);
>> @@ -301,7 +300,7 @@ static int64_t p7ioc_eeh_freeze_status(struct phb
>> *phb, uint64_t pe_number,
>> *pci_error_type = OPAL_EEH_PHB_ERROR;
>> if (severity)
>> *severity = OPAL_EEH_SEV_PHB_DEAD;
>> - goto bail;
>> + return OPAL_SUCCESS;
>> }
>> /* Check fence */
>> @@ -311,7 +310,7 @@ static int64_t p7ioc_eeh_freeze_status(struct phb
>> *phb, uint64_t pe_number,
>> *pci_error_type = OPAL_EEH_PHB_ERROR;
>> if (severity)
>> *severity = OPAL_EEH_SEV_PHB_FENCED;
>> - goto bail;
>> + return OPAL_SUCCESS;
>> }
>> /* Check the PEEV */
>> @@ -345,10 +344,6 @@ static int64_t p7ioc_eeh_freeze_status(struct phb
>> *phb, uint64_t pe_number,
>> else
>> *pci_error_type = OPAL_EEH_PE_DMA_ERROR;
>> - bail:
>> - if (phb_status)
>> - p7ioc_eeh_read_phb_status(p, (struct OpalIoP7IOCPhbErrorData *)
>> - phb_status);
>> return OPAL_SUCCESS;
>> }
>> diff --git a/hw/phb3.c b/hw/phb3.c
>> index 04c8b78..956a6f6 100644
>> --- a/hw/phb3.c
>> +++ b/hw/phb3.c
>> @@ -2747,8 +2747,7 @@ static struct pci_slot *phb3_slot_create(struct
>> phb *phb)
>> static int64_t phb3_eeh_freeze_status(struct phb *phb, uint64_t
>> pe_number,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type,
>> - uint16_t *severity,
>> - uint64_t *phb_status)
>> + uint16_t *severity)
>> {
>> struct phb3 *p = phb_to_phb3(phb);
>> uint64_t peev_bit = PPC_BIT(pe_number & 0x3f);
>> @@ -2773,7 +2772,7 @@ static int64_t phb3_eeh_freeze_status(struct phb
>> *phb, uint64_t pe_number,
>> *pci_error_type = OPAL_EEH_PHB_ERROR;
>> if (severity)
>> *severity = OPAL_EEH_SEV_PHB_FENCED;
>> - goto bail;
>> + return OPAL_SUCCESS;
>> }
>> /* Check the PEEV */
>> @@ -2799,11 +2798,6 @@ static int64_t phb3_eeh_freeze_status(struct
>> phb *phb, uint64_t pe_number,
>> if (pestb & IODA2_PESTB_DMA_STOPPED)
>> *freeze_state |= OPAL_EEH_STOPPED_DMA_FREEZE;
>> -bail:
>> - if (phb_status)
>> - phb3_read_phb_status(p,
>> - (struct OpalIoPhb3ErrorData *)phb_status);
>> -
>> return OPAL_SUCCESS;
>> }
>> diff --git a/hw/phb4.c b/hw/phb4.c
>> index c079764..f2a31ab 100644
>> --- a/hw/phb4.c
>> +++ b/hw/phb4.c
>> @@ -3341,8 +3341,7 @@ static bool phb4_freeze_escalate(uint64_t pesta)
>> static int64_t phb4_eeh_freeze_status(struct phb *phb, uint64_t
>> pe_number,
>> uint8_t *freeze_state,
>> uint16_t *pci_error_type,
>> - uint16_t *severity,
>> - uint64_t *phb_status)
>> + uint16_t *severity)
>> {
>> struct phb4 *p = phb_to_phb4(phb);
>> uint64_t peev_bit = PPC_BIT(pe_number & 0x3f);
>> @@ -3367,7 +3366,7 @@ static int64_t phb4_eeh_freeze_status(struct phb
>> *phb, uint64_t pe_number,
>> *pci_error_type = OPAL_EEH_PHB_ERROR;
>> if (severity)
>> *severity = OPAL_EEH_SEV_PHB_FENCED;
>> - goto bail;
>> + return OPAL_SUCCESS;
>> }
>> /* Check the PEEV */
>> @@ -3401,10 +3400,6 @@ static int64_t phb4_eeh_freeze_status(struct
>> phb *phb, uint64_t pe_number,
>> if (pestb & IODA3_PESTB_DMA_STOPPED)
>> *freeze_state |= OPAL_EEH_STOPPED_DMA_FREEZE;
>> -bail:
>> - if (phb_status)
>> - PHBERR(p, "%s: deprecated PHB status\n", __func__);
>> -
>> return OPAL_SUCCESS;
>> }
>>
>
--
Alexey
More information about the Skiboot
mailing list