Re: [PATCH v5 2/5] powerpc/pseries/eeh: Add RTAS error injection helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] powerpc/pseries/eeh: validate_addr_mask_in_pe() matches unassigned BARs
- [Low] powerpc/pseries/eeh: unused static helpers in eeh_pseries.c fail -Werror build
--

commit 0a875c4e62df7429dee888710961a7609ab3e1b1
Author: Narayana Murty N <nnmlinux@linux.ibm.com>

powerpc/pseries/eeh: Add RTAS error injection helpers

This patch adds helper functions to arch/powerpc/platforms/pseries/eeh_pseries.c
to translate generic EEH error types to RTAS encodings, validate error
injection arguments and PCI bus BAR addresses against the target PE, and
populate the PAPR ibm,errinjct work buffer.

> diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
> index b12ef382fec70..999b3065e09b6 100644
> --- a/arch/powerpc/platforms/pseries/eeh_pseries.c
> +++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
[ ... ]
> @@ -786,6 +789,223 @@ static int pseries_notify_resume(struct eeh_dev *edev)
>  }
>  #endif
[ ... ]
> +static int validate_addr_mask_in_pe(struct eeh_pe *pe, unsigned long addr,
> +				    unsigned long mask)
> +{
[ ... ]
> +	pci_lock_rescan_remove();
> +	eeh_pe_for_each_dev(pe, edev, tmp) {
> +		pdev = eeh_dev_to_pci_dev(edev);
> +		if (!pdev)
> +			continue;
> +
> +		/*
> +		 * Validate against standard BARs (BAR0..BAR5) only.
> +		 * ROM and bridge resources are not IOA bus-error targets.
> +		 */
> +		for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +			res = &pdev->resource[bar];
> +
> +			if (!resource_size(res))
> +				continue;
> +
> +			if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +				continue;
> +
> +			pcibios_resource_to_bus(pdev->bus, &region, res);

[Severity: Medium]
Can validate_addr_mask_in_pe() falsely match unassigned, disabled, or
zero-initialized BARs when called via:

  eeh_pe_inject_err()
    -> pseries_eeh_err_inject()
      -> prepare_errinjct_buffer()
        -> validate_addr_mask_in_pe()

First, resource_size(res) computes res->end - res->start + 1, which
evaluates to 1 rather than 0 when res->start == 0 and res->end == 0
(unlike pci_resource_len(pdev, bar)).

Second, when a standard BAR is sized but unassigned on powerpc, such as in
pcibios_fixup_resources():

  res->end -= res->start;
  res->start = 0;
  res->flags |= IORESOURCE_UNSET;

res->flags retains IORESOURCE_MEM or IORESOURCE_IO alongside
IORESOURCE_UNSET, with a zero-based range [0, size - 1].

When pcibios_resource_to_bus() is called on an IORESOURCE_UNSET resource:

  resource_list_for_each_entry(window, &bridge->windows) {
          if (resource_contains(window->res, res)) {
                  offset = window->offset;
                  break;
          }
  }

  region->start = res->start - offset;
  region->end = res->end - offset;

resource_contains() returns false because it rejects IORESOURCE_UNSET, so
window->offset is not subtracted and region.start is set to 0 with
region.end set to size - 1 (or left as the untranslated CPU resource
address if of_pci_parse_addrs() shifted res by window->offset without
zero-basing it). Any caller-supplied addr in [0, size) (including addr = 0
with a non-zero mask) will then match the unassigned BAR.

Should this check pci_resource_len(pdev, bar) and reject BARs with
IORESOURCE_UNSET | IORESOURCE_DISABLED set in res->flags, similar to
eeh_break_device()?

> +
> +			bus_start = region.start;
> +			bus_len = resource_size(res);
[ ... ]
> +static int validate_errinjct_args(struct eeh_pe *pe, int type, int func,
> +				  unsigned long addr, unsigned long mask)
> +{
> +	if (!pe || !pe->phb)
> +		return -EINVAL;
> +
> +	if (pseries_eeh_type_to_rtas(type) < 0)
> +		return -EINVAL;
> +
> +	if (func < EEH_ERR_FUNC_MIN || func > EEH_ERR_FUNC_MAX)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
[ ... ]
> +static int prepare_errinjct_buffer(void *buf, struct eeh_pe *pe,
> +				   int rtas_err_type, int func,
> +				   unsigned long addr, unsigned long mask)
> +{

[Severity: Low]
Does compiling arch/powerpc/platforms/pseries/eeh_pseries.o at this commit
fail with -Werror=unused-function when CONFIG_PPC_WERROR=y or
CONFIG_WERROR=y is enabled?

Both validate_errinjct_args() and prepare_errinjct_buffer() are defined as
static without __maybe_unused, and pseries_eeh_err_inject() does not call
them until the subsequent patch in the series ("powerpc/pseries/eeh:
Implement RTAS-based EEH error injection").

The commit message notes:

  "pseries_eeh_err_inject() retains the existing MMIO injection body
   unchanged in this patch.  It is replaced by the full RTAS session
   implementation in the next patch.  This keeps the patch bisectable:
   each commit builds and functions correctly independently."

Should validate_errinjct_args() and prepare_errinjct_buffer() be marked
__maybe_unused in this patch (or folded into the next patch) so bisection
does not hit a build failure under -Werror?

> +	__be64 *buf64 = (__be64 *)buf;
> +	__be32 *buf32 = (__be32 *)buf;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019101813.45085-1-nnmlinux@linux.ibm.com?part=2