Re: [PATCH V4 4/5] iommu/hyperv: Only call pviommu init on non-root VMs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommu/hyperv: hyperv_init() overwrites iommu_init without prior check
--

commit ea2b425f9f0c2f00b3af04374abd0211b8229429
Author: Mukesh R <mrathor@linux.microsoft.com>

iommu/hyperv: Only call pviommu init on non-root VMs

This commit restricts setting x86_init.iommu.iommu_init to the Hyper-V
PVIOMMU initialization callback to non-root partitions and renames
hv_iommu_init() to hv_pviommu_init().

>     iommu/hyperv: Only call pviommu init on non-root VMs
>     
>     Next patch introduces root iommu init. Since both can co-exist, set
>     x86 iommu_init to pviommu init only on non-root VMs. Also, rename
>     hv_iommu_init() to hv_pviommu_init(). Lastly, check for null iommu_init
>     before assignement as suggested by one of the pviommu authors.
>     
>     Signed-off-by: Mukesh R <mrathor@linux.microsoft.com>

> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index b90e6f4875335..ea50708c3ae60 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -580,7 +580,8 @@ void __init hyperv_init(void)
>  	x86_init.timers.setup_percpu_clockev = hv_stimer_setup_percpu_clockev;
>  
>  #ifdef CONFIG_HYPERV_PVIOMMU
> -	x86_init.iommu.iommu_init = hv_iommu_init;
> +	if (!hv_root_partition())
> +		x86_init.iommu.iommu_init = hv_pviommu_init;

[Severity: High]
The commit message states:

  "Lastly, check for null iommu_init before assignement as suggested by one
  of the pviommu authors."

However, hyperv_init() in arch/x86/hyperv/hv_init.c only checks
!hv_root_partition() before overwriting x86_init.iommu.iommu_init. Was the
check accidentally omitted from this patch?

Also, x86_init.iommu.iommu_init is initialized to iommu_init_noop rather
than NULL in arch/x86/kernel/x86_init.c:

    .iommu = {
        .iommu_init = iommu_init_noop,
    },

During x86 boot, start_kernel() -> mm_core_init() -> arch_mm_preinit() ->
pci_iommu_alloc() runs before init_IRQ() -> apic_intr_mode_init() ->
x86_platform.apic_post_init() -> hyperv_init(). If an ACPI DMAR or IVRS
table is present on a non-root partition (such as a guest VM with an
emulated Intel VT-d or AMD-Vi vIOMMU, or an L1VH partition where
hv_root_partition() returns false), detect_intel_iommu() in
drivers/iommu/intel/dmar.c or amd_iommu_detect() in
drivers/iommu/amd/init.c sets iommu_detected = 1 and sets
x86_init.iommu.iommu_init to intel_iommu_init or amd_iommu_init.

When hyperv_init() subsequently overwrites x86_init.iommu.iommu_init with
hv_pviommu_init, apic_bsp_setup() -> enable_IR_x2apic() ->
irq_remapping_prepare() still allocates early DMAR/IVRS structures
(dmar_drhd_unit and intel_iommu, or amd_iommu instances, device tables, and
command buffers). Later, when pci_iommu_init() invokes
x86_init.iommu.iommu_init(), hv_pviommu_init() in
drivers/iommu/hyperv/hv-iommu-guest.c sees iommu_detected set:

    if (no_iommu || iommu_detected)
        return -ENODEV;

and returns -ENODEV without running intel_iommu_init() or amd_iommu_init().
Could this leave neither IOMMU initialized while leaking the early-allocated
DMAR/IVRS structures, and should hyperv_init() check !iommu_detected (or
x86_init.iommu.iommu_init == iommu_init_noop) before assigning
hv_pviommu_init?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105020106.3496339-1-mrathor@linux.microsoft.com?part=4