Re: [PATCH v11 2/2] iommu/riscv: create a auxiliary device for HPM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The PMU auxiliary device is destroyed during the devres phase, after the IOMMU hardware and IRQs have already been torn down in the parent driver's remove callback.

Pre-existing issues:
- [High] Missing release barrier when locklessly updating the IOMMU Device Directory Table.
--

commit 93eb137b97dbeb7d61f4f9e3cac367ef538ba9b7
Author: Zong Li <zong.li@sifive.com>

iommu/riscv: create a auxiliary device for HPM

This commit creates an auxiliary device for the hardware performance
monitor (HPM) when supported by the RISC-V IOMMU.

> diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
> --- a/drivers/iommu/riscv/iommu.c
> +++ b/drivers/iommu/riscv/iommu.c

[ ... ]

> @@ -587,6 +591,36 @@ static irqreturn_t riscv_iommu_fltq_process(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * IOMMU Hardware performance monitor
> + */
> +static void riscv_iommu_pmu_id_free(void *data)
> +{
> +	ida_free(&riscv_iommu_pmu_ida, (unsigned long)data);
> +}
> +
> +static int riscv_iommu_hpm_enable(struct riscv_iommu_device *iommu)
> +{
> +	struct auxiliary_device *auxdev;
> +	int id, ret;
> +
> +	id = ida_alloc(&riscv_iommu_pmu_ida, GFP_KERNEL);
> +	if (id < 0)
> +		return id;
> +
> +	ret = devm_add_action_or_reset(iommu->dev, riscv_iommu_pmu_id_free,
> +				       (void *)(unsigned long)id);
> +	if (ret)
> +		return ret;
> +
> +	auxdev = __devm_auxiliary_device_create(iommu->dev, "riscv-iommu",
> +						"pmu", iommu, id);

[Severity: High]
Will this devm-managed auxiliary device be destroyed too late during teardown?

When the driver is unbound (for example, via sysfs), the parent driver's
remove callback in riscv_iommu_platform_remove() is called. This manually
disables the IOMMU and calls platform_device_msi_free_irqs_all() to tear
down MSIs.

Because __devm_auxiliary_device_create() manages the auxiliary device via
devres on the parent device, its destruction and the PMU driver's unbind
happen after the parent remove callback returns.

Could this cause the PMU driver's unbind process to access torn-down MSI
descriptors or disabled hardware when it tries to free its IRQ handler or
stop the PMU?

> +	if (!auxdev)
> +		return -ENODEV;
> +
> +	return 0;
> +}

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a missing
release barrier when locklessly updating the IOMMU Device Directory Table.

Looking at riscv_iommu_get_dc(), a new page is allocated and published
using cmpxchg_relaxed():

        ptr = riscv_iommu_get_pages(iommu, SZ_4K);
        if (!ptr)
                return NULL;

        new = phys_to_ppn(__pa(ptr)) | RISCV_IOMMU_DDTE_V;
        old = cmpxchg_relaxed((unsigned long *)ddtp, ddt, new);

Since cmpxchg_relaxed() lacks release semantics, could the CPU reorder
the page zeroing stores to happen after the pointer is published?

If the IOMMU hardware or another CPU traverses the table concurrently,
is there a risk it might follow the valid pointer and read uninitialized
memory, interpreting it as valid context entries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914035723.2922057-1-zong.li@sifive.com?part=2