* [PATCH v7 5/8] drm/nouveau: Use hmm_range_fault_unlocked_timeout() for SVM faults
From: Stanislav Kinsburskii @ 2026-07-07 19:47 UTC (permalink / raw)
To: airlied, akhilesh, akpm, corbet, dakr, david, decui, haiyangz,
jgg, kees, kys, leon, liam, lizhi.hou, ljs, longli, lyude,
maarten.lankhorst, mamin506, mhocko, mripard, nouveau, ogabbay,
oleg, rppt, shuah, simona, skhan, skinsburskii, surenb,
tzimmermann, vbabka, wei.liu, skinsburskii
Cc: dri-devel, linux-mm, linux-doc, linux-hyperv, linux-kernel,
linux-kselftest, linux-rdma
In-Reply-To: <178345345668.660027.2952911919681614557.stgit@skinsburskii>
nouveau_range_fault() takes mmap_read_lock() only to call
hmm_range_fault(). It also keeps a single HMM_RANGE_DEFAULT_TIMEOUT
deadline across both HMM -EBUSY retries and post-fault
mmu_interval_read_retry() retries.
Use hmm_range_fault_unlocked_timeout() instead. The HMM helper now owns
the mmap lock and refreshes range->notifier_seq for its internal retries.
Nouveau keeps its existing absolute deadline in the outer loop and passes
the remaining jiffies to the helper for each fault attempt, so retries
caused by mmu_interval_read_retry() do not reset the overall retry budget.
Nouveau still validates the interval notifier sequence while holding
svmm->mutex before programming the GPU mapping.
Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
---
drivers/gpu/drm/nouveau/nouveau_svm.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index dcc92131488e..ba93273341af 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -683,15 +683,10 @@ static int nouveau_range_fault(struct nouveau_svmm *svmm,
goto out;
}
- range.notifier_seq = mmu_interval_read_begin(range.notifier);
- mmap_read_lock(mm);
- ret = hmm_range_fault(&range);
- mmap_read_unlock(mm);
- if (ret) {
- if (ret == -EBUSY)
- continue;
+ ret = hmm_range_fault_unlocked_timeout(&range,
+ max_t(long, timeout - jiffies, 1UL));
+ if (ret)
goto out;
- }
mutex_lock(&svmm->mutex);
if (mmu_interval_read_retry(range.notifier,
^ permalink raw reply related
* [PATCH v7 6/8] RDMA/umem: Use hmm_range_fault_unlocked_timeout() for ODP faults
From: Stanislav Kinsburskii @ 2026-07-07 19:47 UTC (permalink / raw)
To: airlied, akhilesh, akpm, corbet, dakr, david, decui, haiyangz,
jgg, kees, kys, leon, liam, lizhi.hou, ljs, longli, lyude,
maarten.lankhorst, mamin506, mhocko, mripard, nouveau, ogabbay,
oleg, rppt, shuah, simona, skhan, skinsburskii, surenb,
tzimmermann, vbabka, wei.liu, skinsburskii
Cc: dri-devel, linux-mm, linux-doc, linux-hyperv, linux-kernel,
linux-kselftest, linux-rdma
In-Reply-To: <178345345668.660027.2952911919681614557.stgit@skinsburskii>
ib_umem_odp_map_dma_and_lock() takes mmap_read_lock() only around
hmm_range_fault(), then retries -EBUSY until HMM_RANGE_DEFAULT_TIMEOUT
expires.
Use hmm_range_fault_unlocked_timeout() instead. The HMM helper now owns
the mmap lock and refreshes range->notifier_seq for its internal retries.
ODP keeps using HMM_RANGE_DEFAULT_TIMEOUT for each HMM fault attempt,
while interval invalidation retries continue to be handled by the existing
outer loop.
ODP still validates the interval notifier sequence while holding umem_mutex
before DMA mapping pages.
Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
---
drivers/infiniband/core/umem_odp.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c
index 404fa1cc3254..9cc21cd762d9 100644
--- a/drivers/infiniband/core/umem_odp.c
+++ b/drivers/infiniband/core/umem_odp.c
@@ -329,7 +329,7 @@ int ib_umem_odp_map_dma_and_lock(struct ib_umem_odp *umem_odp, u64 user_virt,
struct mm_struct *owning_mm = umem_odp->umem.owning_mm;
int pfn_index, dma_index, ret = 0, start_idx;
unsigned int page_shift, hmm_order, pfn_start_idx;
- unsigned long num_pfns, current_seq;
+ unsigned long num_pfns;
struct hmm_range range = {};
unsigned long timeout;
@@ -363,26 +363,18 @@ int ib_umem_odp_map_dma_and_lock(struct ib_umem_odp *umem_odp, u64 user_virt,
}
range.hmm_pfns = &(umem_odp->map.pfn_list[pfn_start_idx]);
- timeout = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
+ timeout = msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
retry:
- current_seq = range.notifier_seq =
- mmu_interval_read_begin(&umem_odp->notifier);
-
- mmap_read_lock(owning_mm);
- ret = hmm_range_fault(&range);
- mmap_read_unlock(owning_mm);
- if (unlikely(ret)) {
- if (ret == -EBUSY && !time_after(jiffies, timeout))
- goto retry;
+ ret = hmm_range_fault_unlocked_timeout(&range, timeout);
+ if (unlikely(ret))
goto out_put_mm;
- }
start_idx = (range.start - ib_umem_start(umem_odp)) >> page_shift;
dma_index = start_idx;
mutex_lock(&umem_odp->umem_mutex);
- if (mmu_interval_read_retry(&umem_odp->notifier, current_seq)) {
+ if (mmu_interval_read_retry(&umem_odp->notifier, range.notifier_seq)) {
mutex_unlock(&umem_odp->umem_mutex);
goto retry;
}
^ permalink raw reply related
* [PATCH v7 7/8] accel/amdxdna: Use hmm_range_fault_unlocked_timeout() for range population
From: Stanislav Kinsburskii @ 2026-07-07 19:47 UTC (permalink / raw)
To: airlied, akhilesh, akpm, corbet, dakr, david, decui, haiyangz,
jgg, kees, kys, leon, liam, lizhi.hou, ljs, longli, lyude,
maarten.lankhorst, mamin506, mhocko, mripard, nouveau, ogabbay,
oleg, rppt, shuah, simona, skhan, skinsburskii, surenb,
tzimmermann, vbabka, wei.liu, skinsburskii
Cc: dri-devel, linux-mm, linux-doc, linux-hyperv, linux-kernel,
linux-kselftest, linux-rdma
In-Reply-To: <178345345668.660027.2952911919681614557.stgit@skinsburskii>
aie2_populate_range() takes mmap_read_lock() only around hmm_range_fault().
It keeps a single HMM_RANGE_DEFAULT_TIMEOUT deadline for the populate pass
and retries -EBUSY until that deadline expires.
Use hmm_range_fault_unlocked_timeout() instead. The HMM helper now owns
the mmap lock and refreshes mapp->range.notifier_seq for its internal
retries. Pass the remaining jiffies from the existing deadline to HMM,
while preserving the driver's existing outer loop for interval invalidation
retries and for selecting the next invalid mapping.
Keep returning -ETIME when the retry budget expires, matching the driver's
existing timeout error convention.
Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
---
drivers/accel/amdxdna/aie2_ctx.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
index e9fbd8c14364..412f812d7c2c 100644
--- a/drivers/accel/amdxdna/aie2_ctx.c
+++ b/drivers/accel/amdxdna/aie2_ctx.c
@@ -1031,22 +1031,11 @@ static int aie2_populate_range(struct amdxdna_gem_obj *abo)
return -EFAULT;
}
- mapp->range.notifier_seq = mmu_interval_read_begin(&mapp->notifier);
- mmap_read_lock(mm);
- ret = hmm_range_fault(&mapp->range);
- mmap_read_unlock(mm);
+ ret = hmm_range_fault_unlocked_timeout(&mapp->range,
+ max_t(long, timeout - jiffies, 1));
if (ret) {
- if (time_after(jiffies, timeout)) {
+ if (ret == -EBUSY)
ret = -ETIME;
- goto put_mm;
- }
-
- if (ret == -EBUSY) {
- amdxdna_umap_put(mapp);
- mmput(mm);
- goto again;
- }
-
goto put_mm;
}
^ permalink raw reply related
* [PATCH v7 8/8] drm/gpusvm: Use hmm_range_fault_unlocked_timeout() for range faults
From: Stanislav Kinsburskii @ 2026-07-07 19:47 UTC (permalink / raw)
To: airlied, akhilesh, akpm, corbet, dakr, david, decui, haiyangz,
jgg, kees, kys, leon, liam, lizhi.hou, ljs, longli, lyude,
maarten.lankhorst, mamin506, mhocko, mripard, nouveau, ogabbay,
oleg, rppt, shuah, simona, skhan, skinsburskii, surenb,
tzimmermann, vbabka, wei.liu, skinsburskii
Cc: dri-devel, linux-mm, linux-doc, linux-hyperv, linux-kernel,
linux-kselftest, linux-rdma
In-Reply-To: <178345345668.660027.2952911919681614557.stgit@skinsburskii>
Several GPU SVM paths take mmap_read_lock() only to call hmm_range_fault(),
then retry -EBUSY until HMM_RANGE_DEFAULT_TIMEOUT expires. Those paths use
MMU interval notifiers whose mm matches the mm that was locked for the HMM
fault.
Use hmm_range_fault_unlocked_timeout() for those faults and pass the
remaining retry budget to HMM. The helper owns mmap_lock acquisition and
refreshes range->notifier_seq internally for each retry, while GPU SVM
keeps its existing driver-lock validation with mmu_interval_read_retry()
after a successful fault.
Leave drm_gpusvm_check_pages() on hmm_range_fault() because that path is
called with the mmap lock already held by its caller.
Signed-off-by: Stanislav Kinsburskii <skinsburskii@gmail.com>
---
drivers/gpu/drm/drm_gpusvm.c | 52 ++++++------------------------------------
1 file changed, 7 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
index 958cb605aedd..2601b793428c 100644
--- a/drivers/gpu/drm/drm_gpusvm.c
+++ b/drivers/gpu/drm/drm_gpusvm.c
@@ -788,22 +788,8 @@ enum drm_gpusvm_scan_result drm_gpusvm_scan_mm(struct drm_gpusvm_range *range,
hmm_range.hmm_pfns = pfns;
retry:
- hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
- mmap_read_lock(range->gpusvm->mm);
-
- while (true) {
- err = hmm_range_fault(&hmm_range);
- if (err == -EBUSY) {
- if (time_after(jiffies, timeout))
- break;
-
- hmm_range.notifier_seq =
- mmu_interval_read_begin(notifier);
- continue;
- }
- break;
- }
- mmap_read_unlock(range->gpusvm->mm);
+ err = hmm_range_fault_unlocked_timeout(&hmm_range,
+ max_t(long, timeout - jiffies, 1));
if (err)
goto err_free;
@@ -1439,21 +1425,8 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
}
hmm_range.hmm_pfns = pfns;
- while (true) {
- mmap_read_lock(mm);
- err = hmm_range_fault(&hmm_range);
- mmap_read_unlock(mm);
-
- if (err == -EBUSY) {
- if (time_after(jiffies, timeout))
- break;
-
- hmm_range.notifier_seq =
- mmu_interval_read_begin(notifier);
- continue;
- }
- break;
- }
+ err = hmm_range_fault_unlocked_timeout(&hmm_range,
+ max_t(long, timeout - jiffies, 1));
mmput(mm);
if (err)
goto err_free;
@@ -1736,24 +1709,13 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm,
return -ENOMEM;
hmm_range.hmm_pfns = pfns;
- while (!time_after(jiffies, timeout)) {
- hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
- if (time_after(jiffies, timeout)) {
- err = -ETIME;
- break;
- }
-
- mmap_read_lock(mm);
- err = hmm_range_fault(&hmm_range);
- mmap_read_unlock(mm);
- if (err != -EBUSY)
- break;
- }
+ err = hmm_range_fault_unlocked_timeout(&hmm_range,
+ max_t(long, timeout - jiffies, 1));
kvfree(pfns);
mmput(mm);
- return err;
+ return err == -EBUSY ? -ETIME : err;
}
EXPORT_SYMBOL_GPL(drm_gpusvm_range_evict);
^ permalink raw reply related
* Re: [PATCH v2 1/4] hyperv: Introduce new hypercall interfaces used by Hyper-V guest IOMMU
From: Mukesh R @ 2026-07-07 21:10 UTC (permalink / raw)
To: Yu Zhang, linux-kernel, linux-hyperv, iommu, linux-pci,
linux-arch
Cc: wei.liu, kys, haiyangz, decui, longli, joro, will, robin.murphy,
bhelgaas, kwilczynski, lpieralisi, mani, robh, arnd, jgg,
mhklinux, jacob.pan, tgopinath, easwar.hariharan
In-Reply-To: <20260702160518.311234-2-zhangyu1@linux.microsoft.com>
On 7/2/26 09:05, Yu Zhang wrote:
> From: Wei Liu <wei.liu@kernel.org>
>
> Hyper-V guest IOMMU is a para-virtualized IOMMU based on hypercalls.
> Introduce the hypercalls used by the child partition to interact with
> this facility.
>
> These hypercalls fall into below categories:
> - Detection and capability: HVCALL_GET_IOMMU_CAPABILITIES is used to
> detect the existence and capabilities of the guest IOMMU.
>
> - Device management: HVCALL_GET_LOGICAL_DEVICE_PROPERTY is used to
> check whether an endpoint device is managed by the guest IOMMU.
>
> - Domain management: A set of hypercalls is provided to handle the
> creation, configuration, and deletion of guest domains, as well as
> the attachment/detachment of endpoint devices to/from those domains.
>
> - IOTLB flushing: HVCALL_FLUSH_DEVICE_DOMAIN is used to ask Hyper-V
> for a domain-selective IOTLB flush (which in its handler may flush
> the device TLB as well).
>
> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> Co-developed-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> Signed-off-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> ---
> include/hyperv/hvgdk_mini.h | 8 +++
> include/hyperv/hvhdk_mini.h | 124 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 132 insertions(+)
>
> diff --git a/include/hyperv/hvgdk_mini.h b/include/hyperv/hvgdk_mini.h
> index 6a4e8b9d570f..5bdbb44da112 100644
> --- a/include/hyperv/hvgdk_mini.h
> +++ b/include/hyperv/hvgdk_mini.h
> @@ -486,10 +486,16 @@ union hv_vp_assist_msr_contents { /* HV_REGISTER_VP_ASSIST_PAGE */
> #define HVCALL_GET_VP_INDEX_FROM_APIC_ID 0x009a
> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
> +#define HVCALL_CREATE_DEVICE_DOMAIN 0x00b1
> +#define HVCALL_ATTACH_DEVICE_DOMAIN 0x00b2
> #define HVCALL_SIGNAL_EVENT_DIRECT 0x00c0
> #define HVCALL_POST_MESSAGE_DIRECT 0x00c1
> #define HVCALL_DISPATCH_VP 0x00c2
> +#define HVCALL_DETACH_DEVICE_DOMAIN 0x00c4
> +#define HVCALL_DELETE_DEVICE_DOMAIN 0x00c5
> #define HVCALL_GET_GPA_PAGES_ACCESS_STATES 0x00c9
> +#define HVCALL_CONFIGURE_DEVICE_DOMAIN 0x00ce
> +#define HVCALL_FLUSH_DEVICE_DOMAIN 0x00d0
> #define HVCALL_ACQUIRE_SPARSE_SPA_PAGE_HOST_ACCESS 0x00d7
> #define HVCALL_RELEASE_SPARSE_SPA_PAGE_HOST_ACCESS 0x00d8
> #define HVCALL_MODIFY_SPARSE_GPA_PAGE_HOST_VISIBILITY 0x00db
> @@ -502,6 +508,8 @@ union hv_vp_assist_msr_contents { /* HV_REGISTER_VP_ASSIST_PAGE */
> #define HVCALL_MMIO_READ 0x0106
> #define HVCALL_MMIO_WRITE 0x0107
> #define HVCALL_DISABLE_HYP_EX 0x010f
> +#define HVCALL_GET_IOMMU_CAPABILITIES 0x0125
> +#define HVCALL_GET_LOGICAL_DEVICE_PROPERTY 0x0127
> #define HVCALL_MAP_STATS_PAGE2 0x0131
>
> /* HV_HYPERCALL_INPUT */
> diff --git a/include/hyperv/hvhdk_mini.h b/include/hyperv/hvhdk_mini.h
> index b4cb2fa26e9b..493608e791b4 100644
> --- a/include/hyperv/hvhdk_mini.h
> +++ b/include/hyperv/hvhdk_mini.h
> @@ -547,4 +547,128 @@ union hv_device_id { /* HV_DEVICE_ID */
> } acpi;
> } __packed;
>
> +/* Device domain types */
> +#define HV_DEVICE_DOMAIN_TYPE_S1 1 /* Stage 1 domain */
> +
> +/* ID for default domain and NULL domain */
> +#define HV_DEVICE_DOMAIN_ID_DEFAULT 0
> +#define HV_DEVICE_DOMAIN_ID_NULL 0xFFFFFFFFULL
> +
> +union hv_device_domain_id {
> + u64 as_uint64;
> + struct {
> + u32 type: 4;
> + u32 reserved: 28;
> + u32 id;
> + } __packed;
> +};
> +
> +struct hv_input_device_domain {
> + u64 partition_id;
> + union hv_input_vtl owner_vtl;
> + u8 padding[7];
> + union hv_device_domain_id domain_id;
> +} __packed;
> +
> +union hv_create_device_domain_flags {
> + u32 as_uint32;
> + struct {
> + u32 forward_progress_required: 1;
> + u32 inherit_owning_vtl: 1;
> + u32 reserved: 30;
> + } __packed;
> +};
> +
> +struct hv_input_create_device_domain {
> + struct hv_input_device_domain device_domain;
> + union hv_create_device_domain_flags create_device_domain_flags;
> +} __packed;
> +
> +struct hv_input_delete_device_domain {
> + struct hv_input_device_domain device_domain;
> +} __packed;
> +
> +struct hv_input_attach_device_domain {
> + struct hv_input_device_domain device_domain;
> + union hv_device_id device_id;
> +} __packed;
> +
> +struct hv_input_detach_device_domain {
> + u64 partition_id;
> + union hv_device_id device_id;
> +} __packed;
> +
> +struct hv_device_domain_settings {
> + struct {
> + /*
> + * Enable translations. If not enabled, all transaction bypass
> + * S1 translations.
> + */
> + u64 translation_enabled: 1;
> + u64 blocked: 1;
> + /*
> + * First stage address translation paging mode:
> + * 0: 4-level paging (default)
> + * 1: 5-level paging
> + */
> + u64 first_stage_paging_mode: 1;
> + u64 reserved: 61;
> + } flags;
> +
> + /* Address of translation table */
> + u64 page_table_root;
> +} __packed;
> +
> +struct hv_input_configure_device_domain {
> + struct hv_input_device_domain device_domain;
> + struct hv_device_domain_settings settings;
> +} __packed;
> +
> +struct hv_input_get_iommu_capabilities {
> + u64 partition_id;
> + u64 reserved;
> +} __packed;
> +
> +struct hv_output_get_iommu_capabilities {
> + u32 size;
> + u16 reserved;
> + u8 max_iova_width;
> + u8 max_pasid_width;
> +
> +#define HV_IOMMU_CAP_PRESENT (1ULL << 0)
> +#define HV_IOMMU_CAP_S2 (1ULL << 1)
> +#define HV_IOMMU_CAP_S1 (1ULL << 2)
> +#define HV_IOMMU_CAP_S1_5LVL (1ULL << 3)
> +#define HV_IOMMU_CAP_PASID (1ULL << 4)
> +#define HV_IOMMU_CAP_ATS (1ULL << 5)
> +#define HV_IOMMU_CAP_PRI (1ULL << 6)
> +
> + u64 iommu_cap;
> + u64 pgsize_bitmap;
> +} __packed;
> +
> +enum hv_logical_device_property_code {
> + HV_LOGICAL_DEVICE_PROPERTY_PVIOMMU = 10,
> +};
> +
> +struct hv_input_get_logical_device_property {
> + u64 partition_id;
> + u64 logical_device_id;
> + /* Takes values from enum hv_logical_device_property_code. */
> + u32 code;
> + u32 reserved;
> +} __packed;
> +
> +struct hv_output_get_logical_device_property {
> +#define HV_DEVICE_IOMMU_ENABLED (1ULL << 0)
> + u64 device_iommu;
> + u64 reserved;
> +} __packed;
> +
> +struct hv_input_flush_device_domain {
> + struct hv_input_device_domain device_domain;
> + u32 flags;
> + u32 reserved;
> +} __packed;
> +
> #endif /* _HV_HVHDK_MINI_H */
Please add tags to all structs, like,
struct hv_input_device_domain { /* HV_INPUT_DEVICE_DOMAIN */
Or just wait for my patches.
Thanks,
-Mukesh
^ permalink raw reply
* Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Mukesh R @ 2026-07-07 21:17 UTC (permalink / raw)
To: Yu Zhang, linux-kernel, linux-hyperv, iommu, linux-pci,
linux-arch
Cc: wei.liu, kys, haiyangz, decui, longli, joro, will, robin.murphy,
bhelgaas, kwilczynski, lpieralisi, mani, robh, arnd, jgg,
mhklinux, jacob.pan, tgopinath, easwar.hariharan
In-Reply-To: <20260702160518.311234-4-zhangyu1@linux.microsoft.com>
On 7/2/26 09:05, Yu Zhang wrote:
> Add a para-virtualized IOMMU driver for Linux guests running on Hyper-V.
> This driver implements stage-1 IO translation within the guest OS.
> It integrates with the Linux IOMMU core, utilizing Hyper-V hypercalls
> for:
> - Capability discovery
> - Domain allocation, configuration, and deallocation
> - Device attachment and detachment
> - IOTLB invalidation
>
> The driver constructs x86-compatible stage-1 IO page tables in the
> guest memory using consolidated IO page table helpers. This allows
> the guest to manage stage-1 translations independently of vendor-
> specific drivers (like Intel VT-d or AMD IOMMU).
>
> Hyper-V consumes this stage-1 IO page table when a device domain is
> created and configured, and nests it with the host's stage-2 IO page
> tables, therefore eliminating the VM exits for guest IOMMU mapping
> operations. For unmapping operations, VM exits to perform the IOTLB
> flush are still unavoidable.
>
> To identify a device in its hypercall interface, the driver looks up the
> logical device ID prefix registered for the device's PCI domain (see the
> logical device ID registry in hv_common.c) and combines it with the PCI
> function number of the endpoint device.
>
> Co-developed-by: Wei Liu <wei.liu@kernel.org>
> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> Co-developed-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> Signed-off-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> ---
> arch/x86/hyperv/hv_init.c | 4 +
> arch/x86/include/asm/mshyperv.h | 4 +
> drivers/iommu/Kconfig | 1 +
> drivers/iommu/hyperv/Kconfig | 16 +
> drivers/iommu/hyperv/Makefile | 1 +
> drivers/iommu/hyperv/iommu.c | 620 ++++++++++++++++++++++++++++++++
> drivers/iommu/hyperv/iommu.h | 51 +++
> 7 files changed, 697 insertions(+)
> create mode 100644 drivers/iommu/hyperv/Kconfig
> create mode 100644 drivers/iommu/hyperv/iommu.c
> create mode 100644 drivers/iommu/hyperv/iommu.h
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 55a8b6de2865..094f9f7ddb72 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -578,6 +578,10 @@ void __init hyperv_init(void)
> old_setup_percpu_clockev = x86_init.timers.setup_percpu_clockev;
> x86_init.timers.setup_percpu_clockev = hv_stimer_setup_percpu_clockev;
>
> +#ifdef CONFIG_HYPERV_PVIOMMU
> + x86_init.iommu.iommu_init = hv_iommu_init;
> +#endif
> +
> hv_apic_init();
>
> x86_init.pci.arch_init = hv_pci_init;
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index f64393e853ee..20d947c2c758 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -313,6 +313,10 @@ static inline void mshv_vtl_return_hypercall(void) {}
> static inline void __mshv_vtl_return_call(struct mshv_vtl_cpu_context *vtl0) {}
> #endif
>
> +#ifdef CONFIG_HYPERV_PVIOMMU
> +int __init hv_iommu_init(void);
> +#endif
> +
> #include <asm-generic/mshyperv.h>
>
> #endif
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 6e07bd69467a..0d128f377929 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -195,6 +195,7 @@ config MSM_IOMMU
> source "drivers/iommu/amd/Kconfig"
> source "drivers/iommu/arm/Kconfig"
> source "drivers/iommu/intel/Kconfig"
> +source "drivers/iommu/hyperv/Kconfig"
> source "drivers/iommu/iommufd/Kconfig"
> source "drivers/iommu/riscv/Kconfig"
>
> diff --git a/drivers/iommu/hyperv/Kconfig b/drivers/iommu/hyperv/Kconfig
> new file mode 100644
> index 000000000000..8b6abbaaf9b8
> --- /dev/null
> +++ b/drivers/iommu/hyperv/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +# HyperV paravirtualized IOMMU support
> +config HYPERV_PVIOMMU
> + bool "Microsoft Hypervisor para-virtualized IOMMU support"
> + depends on X86_64 && HYPERV
> + select IOMMU_API
> + select GENERIC_PT
> + select IOMMU_PT
> + select IOMMU_PT_X86_64
> + select IOMMU_IOVA
> + default HYPERV
> + help
> + Para-virtualized IOMMU driver for Linux guests running on
> + Microsoft Hyper-V. Provides DMA remapping and IOTLB
> + flush support to enable DMA isolation for devices
> + assigned to the guest.
> diff --git a/drivers/iommu/hyperv/Makefile b/drivers/iommu/hyperv/Makefile
> index 6ef0ef97f3dd..fefb409d976b 100644
> --- a/drivers/iommu/hyperv/Makefile
> +++ b/drivers/iommu/hyperv/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_IRQ_REMAP) += hv-irq-remap-x86.o
> +obj-$(CONFIG_HYPERV_PVIOMMU) += iommu.o
> diff --git a/drivers/iommu/hyperv/iommu.c b/drivers/iommu/hyperv/iommu.c
> new file mode 100644
> index 000000000000..254136946404
> --- /dev/null
> +++ b/drivers/iommu/hyperv/iommu.c
> @@ -0,0 +1,620 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Hyper-V IOMMU driver.
> + *
> + * Copyright (C) 2019, 2024-2026 Microsoft, Inc.
> + */
> +
> +#define pr_fmt(fmt) "Hyper-V pvIOMMU: " fmt
> +#define dev_fmt(fmt) pr_fmt(fmt)
> +
> +#include <linux/iommu.h>
> +#include <linux/pci.h>
> +#include <linux/dma-map-ops.h>
> +#include <linux/generic_pt/iommu.h>
> +#include <linux/pci-ats.h>
> +
> +#include <asm/iommu.h>
> +#include <asm/hypervisor.h>
> +#include <asm/mshyperv.h>
> +
> +#include "iommu.h"
> +#include "../iommu-pages.h"
> +
> +struct hv_iommu_dev *hv_iommu_device;
> +
> +/*
> + * Identity and blocking domains are static singletons: identity is a 1:1
> + * passthrough with no page table, blocking rejects all DMA. Neither holds
> + * per-IOMMU state, so one instance suffices even with multiple vIOMMUs.
> + */
> +static const struct iommu_domain_ops hv_iommu_identity_domain_ops;
> +static const struct iommu_domain_ops hv_iommu_blocking_domain_ops;
> +static struct iommu_ops hv_iommu_ops;
> +
> +static struct hv_iommu_domain hv_identity_domain = {
> + .domain = {
> + .type = IOMMU_DOMAIN_IDENTITY,
> + .ops = &hv_iommu_identity_domain_ops,
> + .owner = &hv_iommu_ops,
> + },
> +};
> +static struct hv_iommu_domain hv_blocking_domain = {
> + .domain = {
> + .type = IOMMU_DOMAIN_BLOCKED,
> + .ops = &hv_iommu_blocking_domain_ops,
> + .owner = &hv_iommu_ops,
> + },
> +};
> +
> +static inline bool hv_iommu_present(u64 cap)
> +{
> + return cap & HV_IOMMU_CAP_PRESENT;
> +}
> +
> +static inline bool hv_iommu_s1_domain_supported(u64 cap)
> +{
> + return cap & HV_IOMMU_CAP_S1;
> +}
> +
> +static inline bool hv_iommu_5lvl_supported(u64 cap)
> +{
> + return cap & HV_IOMMU_CAP_S1_5LVL;
> +}
> +
> +static inline bool hv_iommu_ats_supported(u64 cap)
> +{
> + return cap & HV_IOMMU_CAP_ATS;
> +}
> +
> +static int hv_create_device_domain(struct hv_iommu_domain *hv_domain, u32 domain_stage)
99.99% of our code is80 cols, lets keep that way. someday we can change
all in one shot, until then please avoid line wraps here and below in
ida_free(), hv_iommu_get_logical_device_property, etc...
Thanks,
-Mukesh
> +{
> + int ret;
> + u64 status;
> + unsigned long flags;
> + struct hv_input_create_device_domain *input;
> +
> + ret = ida_alloc_range(&hv_iommu_device->domain_ids,
> + hv_iommu_device->first_domain, hv_iommu_device->last_domain,
> + GFP_KERNEL);
> + if (ret < 0)
> + return ret;
> +
> + hv_domain->device_domain.partition_id = HV_PARTITION_ID_SELF;
> + hv_domain->device_domain.domain_id.type = domain_stage;
> + hv_domain->device_domain.domain_id.id = ret;
> + hv_domain->hv_iommu = hv_iommu_device;
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->device_domain = hv_domain->device_domain;
> + input->create_device_domain_flags.forward_progress_required = 1;
> + input->create_device_domain_flags.inherit_owning_vtl = 0;
> + status = hv_do_hypercall(HVCALL_CREATE_DEVICE_DOMAIN, input, NULL);
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status)) {
> + pr_err("HVCALL_CREATE_DEVICE_DOMAIN failed, status %lld\n", status);
> + ida_free(&hv_iommu_device->domain_ids, hv_domain->device_domain.domain_id.id);
> + }
> +
> + return hv_result_to_errno(status);
> +}
> +
> +static void hv_delete_device_domain(struct hv_iommu_domain *hv_domain)
> +{
> + u64 status;
> + unsigned long flags;
> + struct hv_input_delete_device_domain *input;
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->device_domain = hv_domain->device_domain;
> + status = hv_do_hypercall(HVCALL_DELETE_DEVICE_DOMAIN, input, NULL);
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status))
> + pr_err("HVCALL_DELETE_DEVICE_DOMAIN failed, status %lld\n", status);
> +
> + ida_free(&hv_domain->hv_iommu->domain_ids, hv_domain->device_domain.domain_id.id);
> +}
> +
> +static bool hv_iommu_capable(struct device *dev, enum iommu_cap cap)
> +{
> + switch (cap) {
> + case IOMMU_CAP_CACHE_COHERENCY:
> + return true;
> + case IOMMU_CAP_DEFERRED_FLUSH:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static void hv_flush_device_domain(struct hv_iommu_domain *hv_domain)
> +{
> + u64 status;
> + unsigned long flags;
> + struct hv_input_flush_device_domain *input;
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->device_domain = hv_domain->device_domain;
> + status = hv_do_hypercall(HVCALL_FLUSH_DEVICE_DOMAIN, input, NULL);
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status))
> + pr_err("HVCALL_FLUSH_DEVICE_DOMAIN failed, status %lld\n", status);
> +}
> +
> +static int hv_iommu_attach_dev(struct iommu_domain *domain, struct device *dev,
> + struct iommu_domain *old)
> +{
> + u64 status;
> + u32 prefix;
> + unsigned long flags;
> + struct pci_dev *pdev;
> + struct hv_input_attach_device_domain *input;
> + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> + struct hv_iommu_domain *hv_domain = to_hv_iommu_domain(domain);
> + int ret;
> +
> + if (vdev->hv_domain == hv_domain)
> + return 0;
> +
> + pdev = to_pci_dev(dev);
> + dev_dbg(dev, "attaching to domain %d\n",
> + hv_domain->device_domain.domain_id.id);
> +
> + ret = hv_iommu_lookup_logical_dev_id(pci_domain_nr(pdev->bus), &prefix);
> + if (ret) {
> + dev_err(&pdev->dev, "no IOMMU registration for vPCI bus\n");
> + return ret;
> + }
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->device_domain = hv_domain->device_domain;
> + input->device_id.as_uint64 = (u64)prefix | PCI_FUNC(pdev->devfn);
> + status = hv_do_hypercall(HVCALL_ATTACH_DEVICE_DOMAIN, input, NULL);
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status))
> + pr_err("HVCALL_ATTACH_DEVICE_DOMAIN failed, status %lld\n", status);
> + else
> + vdev->hv_domain = hv_domain;
> +
> + return hv_result_to_errno(status);
> +}
> +
> +static int hv_iommu_blocking_attach_dev(struct iommu_domain *domain,
> + struct device *dev,
> + struct iommu_domain *old)
> +{
> + int ret = hv_iommu_attach_dev(domain, dev, old);
> +
> + /*
> + * Attaching to the blocking domain only asks the hypervisor to
> + * disable translation and IOPF for the device, so it cannot fail
> + * unless there is a driver or hypervisor bug. Return the hypercall
> + * status rather than 0 so that a failure on the DMA ownership claim
> + * path (VFIO/iommufd) fails the claim instead of leaving the device
> + * unblocked. WARN since such a failure indicates a bug.
> + */
> + WARN_ON(ret);
> + return ret;
> +}
> +
> +static int hv_iommu_get_logical_device_property(struct device *dev,
> + u32 code,
> + struct hv_output_get_logical_device_property *property)
> +{
> + u64 status;
> + u32 prefix;
> + unsigned long flags;
> + int ret;
> + struct pci_dev *pdev = to_pci_dev(dev);
> + struct hv_input_get_logical_device_property *input;
> + struct hv_output_get_logical_device_property *output;
> +
> + ret = hv_iommu_lookup_logical_dev_id(pci_domain_nr(pdev->bus), &prefix);
> + if (ret)
> + return ret;
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + output = (struct hv_output_get_logical_device_property *)(input + 1);
> + memset(input, 0, sizeof(*input));
> + input->partition_id = HV_PARTITION_ID_SELF;
> + input->logical_device_id = (u64)prefix | PCI_FUNC(pdev->devfn);
> + input->code = code;
> + status = hv_do_hypercall(HVCALL_GET_LOGICAL_DEVICE_PROPERTY, input, output);
> + *property = *output;
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status))
> + pr_err("HVCALL_GET_LOGICAL_DEVICE_PROPERTY failed, status %lld\n", status);
> +
> + return hv_result_to_errno(status);
> +}
> +
> +static struct iommu_device *hv_iommu_probe_device(struct device *dev)
> +{
> + struct pci_dev *pdev;
> + struct hv_iommu_endpoint *vdev;
> + struct hv_output_get_logical_device_property device_iommu_property = {0};
> +
> + if (!dev_is_pci(dev))
> + return ERR_PTR(-ENODEV);
> +
> + pdev = to_pci_dev(dev);
> +
> + if (hv_iommu_get_logical_device_property(dev,
> + HV_LOGICAL_DEVICE_PROPERTY_PVIOMMU,
> + &device_iommu_property) ||
> + !(device_iommu_property.device_iommu & HV_DEVICE_IOMMU_ENABLED))
> + return ERR_PTR(-ENODEV);
> +
> + vdev = kzalloc_obj(*vdev, GFP_KERNEL);
> + if (!vdev)
> + return ERR_PTR(-ENOMEM);
> +
> + vdev->dev = dev;
> + vdev->hv_iommu = hv_iommu_device;
> + dev_iommu_priv_set(dev, vdev);
> +
> + if (hv_iommu_ats_supported(hv_iommu_device->cap) &&
> + pci_ats_supported(pdev))
> + pci_enable_ats(pdev, __ffs(hv_iommu_device->pgsize_bitmap));
> +
> + return &vdev->hv_iommu->iommu;
> +}
> +
> +static void hv_iommu_release_device(struct device *dev)
> +{
> + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + if (pdev->ats_enabled)
> + pci_disable_ats(pdev);
> +
> + dev_iommu_priv_set(dev, NULL);
> +
> + kfree(vdev);
> +}
> +
> +static struct iommu_group *hv_iommu_device_group(struct device *dev)
> +{
> + if (dev_is_pci(dev))
> + return pci_device_group(dev);
> +
> + WARN_ON_ONCE(1);
> + return generic_device_group(dev);
> +}
> +
> +static int hv_configure_device_domain(struct hv_iommu_domain *hv_domain, u32 domain_type)
> +{
> + u64 status;
> + unsigned long flags;
> + struct pt_iommu_x86_64_hw_info pt_info;
> + struct hv_input_configure_device_domain *input;
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + memset(input, 0, sizeof(*input));
> + input->device_domain = hv_domain->device_domain;
> + input->settings.flags.blocked = (domain_type == IOMMU_DOMAIN_BLOCKED);
> + /*
> + * Clearing translation_enabled bypasses translation (DMA uses the GPA
> + * directly), which only suits identity. The hypervisor requires paging
> + * and blocked domains to keep it set.
> + */
> + input->settings.flags.translation_enabled = (domain_type != IOMMU_DOMAIN_IDENTITY);
> +
> + if (domain_type & __IOMMU_DOMAIN_PAGING) {
> + pt_iommu_x86_64_hw_info(&hv_domain->pt_iommu_x86_64, &pt_info);
> + input->settings.page_table_root = pt_info.gcr3_pt;
> + input->settings.flags.first_stage_paging_mode =
> + pt_info.levels == 5;
> + }
> + status = hv_do_hypercall(HVCALL_CONFIGURE_DEVICE_DOMAIN, input, NULL);
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status))
> + pr_err("HVCALL_CONFIGURE_DEVICE_DOMAIN failed, status %lld\n", status);
> +
> + return hv_result_to_errno(status);
> +}
> +
> +static int __init hv_initialize_static_domains(void)
> +{
> + int ret;
> + struct hv_iommu_domain *hv_domain;
> +
> + /* Default stage-1 identity domain */
> + hv_domain = &hv_identity_domain;
> +
> + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> + if (ret)
> + return ret;
> +
> + ret = hv_configure_device_domain(hv_domain, IOMMU_DOMAIN_IDENTITY);
> + if (ret)
> + goto delete_identity_domain;
> +
> + /* Default stage-1 blocked domain */
> + hv_domain = &hv_blocking_domain;
> +
> + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> + if (ret)
> + goto delete_identity_domain;
> +
> + ret = hv_configure_device_domain(hv_domain, IOMMU_DOMAIN_BLOCKED);
> + if (ret)
> + goto delete_blocked_domain;
> +
> + return 0;
> +
> +delete_blocked_domain:
> + hv_delete_device_domain(&hv_blocking_domain);
> +delete_identity_domain:
> + hv_delete_device_domain(&hv_identity_domain);
> + return ret;
> +}
> +
> +/* x86 architectural MSI address range */
> +#define INTERRUPT_RANGE_START (0xfee00000)
> +#define INTERRUPT_RANGE_END (0xfeefffff)
> +static void hv_iommu_get_resv_regions(struct device *dev,
> + struct list_head *head)
> +{
> + struct iommu_resv_region *region;
> +
> + region = iommu_alloc_resv_region(INTERRUPT_RANGE_START,
> + INTERRUPT_RANGE_END - INTERRUPT_RANGE_START + 1,
> + 0, IOMMU_RESV_MSI, GFP_KERNEL);
> + if (!region)
> + return;
> +
> + list_add_tail(®ion->list, head);
> +}
> +
> +static void hv_iommu_flush_iotlb_all(struct iommu_domain *domain)
> +{
> + hv_flush_device_domain(to_hv_iommu_domain(domain));
> +}
> +
> +static void hv_iommu_iotlb_sync(struct iommu_domain *domain,
> + struct iommu_iotlb_gather *iotlb_gather)
> +{
> + hv_flush_device_domain(to_hv_iommu_domain(domain));
> +
> + iommu_put_pages_list(&iotlb_gather->freelist);
> +}
> +
> +static void hv_iommu_paging_domain_free(struct iommu_domain *domain)
> +{
> + struct hv_iommu_domain *hv_domain = to_hv_iommu_domain(domain);
> +
> + /* Free all remaining mappings */
> + pt_iommu_deinit(&hv_domain->pt_iommu);
> +
> + hv_delete_device_domain(hv_domain);
> +
> + kfree(hv_domain);
> +}
> +
> +static const struct iommu_domain_ops hv_iommu_identity_domain_ops = {
> + .attach_dev = hv_iommu_attach_dev,
> +};
> +
> +static const struct iommu_domain_ops hv_iommu_blocking_domain_ops = {
> + .attach_dev = hv_iommu_blocking_attach_dev,
> +};
> +
> +static const struct iommu_domain_ops hv_iommu_paging_domain_ops = {
> + .attach_dev = hv_iommu_attach_dev,
> + IOMMU_PT_DOMAIN_OPS(x86_64),
> + .flush_iotlb_all = hv_iommu_flush_iotlb_all,
> + .iotlb_sync = hv_iommu_iotlb_sync,
> + .free = hv_iommu_paging_domain_free,
> +};
> +
> +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct device *dev)
> +{
> + int ret;
> + struct hv_iommu_domain *hv_domain;
> + struct pt_iommu_x86_64_cfg cfg = {};
> +
> + hv_domain = kzalloc_obj(*hv_domain, GFP_KERNEL);
> + if (!hv_domain)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> + if (ret)
> + goto err_free;
> +
> + hv_domain->pt_iommu.nid = dev_to_node(dev);
> +
> + cfg.common.hw_max_vasz_lg2 = hv_iommu_device->max_iova_width;
> + cfg.common.hw_max_oasz_lg2 = 52;
> + cfg.top_level = (hv_iommu_device->max_iova_width > 48) ? 4 : 3;
> +
> + ret = pt_iommu_x86_64_init(&hv_domain->pt_iommu_x86_64, &cfg, GFP_KERNEL);
> + if (ret)
> + goto err_delete_domain;
> +
> + /* Constrain to page sizes the hypervisor supports */
> + hv_domain->domain.pgsize_bitmap &= hv_iommu_device->pgsize_bitmap;
> +
> + hv_domain->domain.ops = &hv_iommu_paging_domain_ops;
> +
> + ret = hv_configure_device_domain(hv_domain, __IOMMU_DOMAIN_PAGING);
> + if (ret)
> + goto err_pt_deinit;
> +
> + return &hv_domain->domain;
> +
> +err_pt_deinit:
> + pt_iommu_deinit(&hv_domain->pt_iommu);
> +err_delete_domain:
> + hv_delete_device_domain(hv_domain);
> +err_free:
> + kfree(hv_domain);
> + return ERR_PTR(ret);
> +}
> +
> +static struct iommu_ops hv_iommu_ops = {
> + .capable = hv_iommu_capable,
> + .domain_alloc_paging = hv_iommu_domain_alloc_paging,
> + .probe_device = hv_iommu_probe_device,
> + .release_device = hv_iommu_release_device,
> + .device_group = hv_iommu_device_group,
> + .get_resv_regions = hv_iommu_get_resv_regions,
> + .owner = THIS_MODULE,
> + .identity_domain = &hv_identity_domain.domain,
> + .blocked_domain = &hv_blocking_domain.domain,
> + .release_domain = &hv_blocking_domain.domain,
> +};
> +
> +static int hv_iommu_detect(struct hv_output_get_iommu_capabilities *hv_iommu_cap)
> +{
> + u64 status;
> + unsigned long flags;
> + struct hv_input_get_iommu_capabilities *input;
> + struct hv_output_get_iommu_capabilities *output;
> +
> + local_irq_save(flags);
> +
> + input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> + output = (struct hv_output_get_iommu_capabilities *)(input + 1);
> + memset(input, 0, sizeof(*input));
> + input->partition_id = HV_PARTITION_ID_SELF;
> + status = hv_do_hypercall(HVCALL_GET_IOMMU_CAPABILITIES, input, output);
> + *hv_iommu_cap = *output;
> +
> + local_irq_restore(flags);
> +
> + if (!hv_result_success(status))
> + pr_err("HVCALL_GET_IOMMU_CAPABILITIES failed, status %lld\n", status);
> +
> + return hv_result_to_errno(status);
> +}
> +
> +static void __init hv_init_iommu_device(struct hv_iommu_dev *hv_iommu,
> + struct hv_output_get_iommu_capabilities *hv_iommu_cap)
> +{
> + ida_init(&hv_iommu->domain_ids);
> +
> + hv_iommu->cap = hv_iommu_cap->iommu_cap;
> + hv_iommu->max_iova_width = hv_iommu_cap->max_iova_width;
> + if (!hv_iommu_5lvl_supported(hv_iommu->cap) &&
> + hv_iommu->max_iova_width > 48) {
> + pr_info("5-level paging not supported, limiting iova width to 48.\n");
> + hv_iommu->max_iova_width = 48;
> + }
> +
> + hv_iommu->geometry = (struct iommu_domain_geometry) {
> + .aperture_start = 0,
> + .aperture_end = (((u64)1) << hv_iommu->max_iova_width) - 1,
> + .force_aperture = true,
> + };
> +
> + hv_iommu->first_domain = HV_DEVICE_DOMAIN_ID_DEFAULT + 1;
> + hv_iommu->last_domain = HV_DEVICE_DOMAIN_ID_NULL - 1;
> + hv_iommu->pgsize_bitmap = hv_iommu_cap->pgsize_bitmap;
> + hv_iommu_device = hv_iommu;
> +}
> +
> +int __init hv_iommu_init(void)
> +{
> + int ret = 0;
> + struct hv_iommu_dev *hv_iommu = NULL;
> + struct hv_output_get_iommu_capabilities hv_iommu_cap = {0};
> +
> + if (no_iommu || iommu_detected)
> + return -ENODEV;
> +
> + if (!hv_is_hyperv_initialized())
> + return -ENODEV;
> +
> + ret = hv_iommu_detect(&hv_iommu_cap);
> + if (ret) {
> + pr_err("HVCALL_GET_IOMMU_CAPABILITIES failed: %d\n", ret);
> + return -ENODEV;
> + }
> +
> + if (!hv_iommu_present(hv_iommu_cap.iommu_cap) ||
> + !hv_iommu_s1_domain_supported(hv_iommu_cap.iommu_cap)) {
> + pr_err("IOMMU capabilities not sufficient: cap=0x%llx\n",
> + hv_iommu_cap.iommu_cap);
> + return -ENODEV;
> + }
> +
> + /*
> + * The page table code only maps x86 page sizes (4K/2M/1G); require the
> + * hypervisor to advertise a non-empty subset of exactly those.
> + */
> + if (!hv_iommu_cap.pgsize_bitmap ||
> + (hv_iommu_cap.pgsize_bitmap & ~(u64)(SZ_4K | SZ_2M | SZ_1G))) {
> + pr_err("unsupported page sizes: pgsize_bitmap=0x%llx\n",
> + hv_iommu_cap.pgsize_bitmap);
> + return -ENODEV;
> + }
> +
> + iommu_detected = 1;
> + pci_request_acs();
> +
> + hv_iommu = kzalloc_obj(*hv_iommu, GFP_KERNEL);
> + if (!hv_iommu)
> + return -ENOMEM;
> +
> + hv_init_iommu_device(hv_iommu, &hv_iommu_cap);
> +
> + ret = hv_initialize_static_domains();
> + if (ret) {
> + pr_err("static domains init failed: %d\n", ret);
> + goto err_free;
> + }
> +
> + ret = iommu_device_sysfs_add(&hv_iommu->iommu, NULL, NULL, "%s", "hv-iommu");
> + if (ret) {
> + pr_err("iommu_device_sysfs_add failed: %d\n", ret);
> + goto err_delete_static_domains;
> + }
> +
> + ret = iommu_device_register(&hv_iommu->iommu, &hv_iommu_ops, NULL);
> + if (ret) {
> + pr_err("iommu_device_register failed: %d\n", ret);
> + goto err_sysfs_remove;
> + }
> +
> + pr_info("successfully initialized\n");
> + return 0;
> +
> +err_sysfs_remove:
> + iommu_device_sysfs_remove(&hv_iommu->iommu);
> +err_delete_static_domains:
> + hv_delete_device_domain(&hv_blocking_domain);
> + hv_delete_device_domain(&hv_identity_domain);
> +err_free:
> + kfree(hv_iommu);
> + return ret;
> +}
> diff --git a/drivers/iommu/hyperv/iommu.h b/drivers/iommu/hyperv/iommu.h
> new file mode 100644
> index 000000000000..3a9f40fa2403
> --- /dev/null
> +++ b/drivers/iommu/hyperv/iommu.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Hyper-V IOMMU driver.
> + *
> + * Copyright (C) 2024-2025, Microsoft, Inc.
> + *
> + */
> +
> +#ifndef _HYPERV_IOMMU_H
> +#define _HYPERV_IOMMU_H
> +
> +struct hv_iommu_dev {
> + struct iommu_device iommu;
> + struct ida domain_ids;
> +
> + /* Device configuration */
> + u8 max_iova_width;
> + u8 max_pasid_width;
> + u64 cap;
> + u64 pgsize_bitmap;
> +
> + struct iommu_domain_geometry geometry;
> + u64 first_domain;
> + u64 last_domain;
> +};
> +
> +struct hv_iommu_domain {
> + union {
> + struct iommu_domain domain;
> + struct pt_iommu pt_iommu;
> + struct pt_iommu_x86_64 pt_iommu_x86_64;
> + };
> + struct hv_iommu_dev *hv_iommu;
> + struct hv_input_device_domain device_domain;
> + u64 pgsize_bitmap;
> +};
> +
> +PT_IOMMU_CHECK_DOMAIN(struct hv_iommu_domain, pt_iommu, domain);
> +PT_IOMMU_CHECK_DOMAIN(struct hv_iommu_domain, pt_iommu_x86_64.iommu, domain);
> +
> +struct hv_iommu_endpoint {
> + struct device *dev;
> + struct hv_iommu_dev *hv_iommu;
> + struct hv_iommu_domain *hv_domain;
> +};
> +
> +#define to_hv_iommu_domain(d) \
> + container_of(d, struct hv_iommu_domain, domain)
> +
> +#endif /* _HYPERV_IOMMU_H */
^ permalink raw reply
* [PATCH] mshv: fix fd leak in mshv_ioctl_create_vtl()
From: Yi Xie @ 2026-07-08 1:28 UTC (permalink / raw)
To: kys, haiyangz, wei.liu; +Cc: linux-hyperv, linux-kernel, Yi Xie
put_unused_fd() if anon_inode_getfile() fails.
Signed-off-by: Yi Xie <xieyi@kylinos.cn>
---
drivers/hv/mshv_vtl_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
index 0d3d4161974f..897a41b08d02 100644
--- a/drivers/hv/mshv_vtl_main.c
+++ b/drivers/hv/mshv_vtl_main.c
@@ -129,6 +129,7 @@ mshv_ioctl_create_vtl(void __user *user_arg, struct device *module_dev)
file = anon_inode_getfile("mshv_vtl", &mshv_vtl_fops,
vtl, O_RDWR);
if (IS_ERR(file)) {
+ put_unused_fd(fd);
kfree(vtl);
return PTR_ERR(file);
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] mshv: fix fd leak in mshv_ioctl_create_vtl()
From: sashiko-bot @ 2026-07-08 1:42 UTC (permalink / raw)
To: Yi Xie; +Cc: linux-hyperv
In-Reply-To: <20260708012852.36824-1-xieyi@kylinos.cn>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] The `mshv_vtl_fault` mmap fault handler fails to bounds-check the user-controlled `cpu` index against `nr_cpu_ids`.
--
--- Patch [1]: [PATCH] mshv: fix fd leak in mshv_ioctl_create_vtl() ---
commit 2e3734b84c858c82d0ec893b4137e370673f6928
Author: Yi Xie <xieyi@kylinos.cn>
mshv: fix fd leak in mshv_ioctl_create_vtl()
This commit addresses a file descriptor leak in mshv_ioctl_create_vtl().
It ensures put_unused_fd() is called when anon_inode_getfile() fails,
freeing the allocated file descriptor before returning an error.
> diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
> index 0d3d4161974f8..897a41b08d026 100644
> --- a/drivers/hv/mshv_vtl_main.c
> +++ b/drivers/hv/mshv_vtl_main.c
> @@ -129,6 +129,7 @@ mshv_ioctl_create_vtl(void __user *user_arg, struct device *module_dev)
> file = anon_inode_getfile("mshv_vtl", &mshv_vtl_fops,
> vtl, O_RDWR);
> if (IS_ERR(file)) {
> + put_unused_fd(fd);
> kfree(vtl);
> return PTR_ERR(file);
> }
[Severity: Critical]
This is a pre-existing issue, but while reviewing this file I noticed an
insufficient bounds check in the mmap fault handler.
Does this missing validation allow an out-of-bounds pointer arithmetic
regression?
drivers/hv/mshv_vtl_main.c:mshv_vtl_fault() {
struct page *page;
int cpu = vmf->pgoff & MSHV_PG_OFF_CPU_MASK;
int real_off = vmf->pgoff >> MSHV_REAL_OFF_SHIFT;
if (!cpu_online(cpu))
return VM_FAULT_SIGBUS;
A VMA with a large offset can cause vmf->pgoff to yield an arbitrary
cpu index. The only validation is cpu_online(cpu), which uses an
unbounds-checked test_bit() against cpu_online_mask.
If the out-of-bounds read hits a set bit, the check passes. Execution then
proceeds to mshv_vtl_cpu_run(cpu), calling per_cpu_ptr() which reads from
the __per_cpu_offset array out-of-bounds.
The resulting garbage pointer is passed to virt_to_page(), and get_page()
incorrectly increments a reference count, potentially mapping arbitrary
physical memory into userspace.
Should this code check the cpu index against nr_cpu_ids before passing it
to cpu_online()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708012852.36824-1-xieyi@kylinos.cn?part=1
^ permalink raw reply
* RE: [PATCH v2 2/4] Drivers: hv: Add logical device ID registry for vPCI devices
From: Michael Kelley @ 2026-07-08 2:52 UTC (permalink / raw)
To: Yu Zhang, linux-kernel@vger.kernel.org,
linux-hyperv@vger.kernel.org, iommu@lists.linux.dev,
linux-pci@vger.kernel.org, linux-arch@vger.kernel.org
Cc: wei.liu@kernel.org, kys@microsoft.com, haiyangz@microsoft.com,
decui@microsoft.com, longli@microsoft.com, joro@8bytes.org,
will@kernel.org, robin.murphy@arm.com, bhelgaas@google.com,
kwilczynski@kernel.org, lpieralisi@kernel.org, mani@kernel.org,
robh@kernel.org, arnd@arndb.de, jgg@ziepe.ca, Michael Kelley,
jacob.pan@linux.microsoft.com, tgopinath@linux.microsoft.com,
easwar.hariharan@linux.microsoft.com, mrathor@linux.microsoft.com
In-Reply-To: <20260702160518.311234-3-zhangyu1@linux.microsoft.com>
From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Thursday, July 2, 2026 9:05 AM
>
> From: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
>
> Hyper-V identifies each PCI pass-thru device by a logical device ID in
> its hypercall interface. This ID consists of a per-bus prefix, derived
> from the VMBus device instance GUID, combined with the PCI function
> number of the endpoint device.
>
> Add a small registry in hv_common.c that maps a PCI domain number to its
> logical device ID prefix. The vPCI bus driver (pci-hyperv) registers the
> prefix when a bus is probed and unregisters it when the bus is removed.
> Consumers such as the para-virtualized IOMMU driver look up the prefix
> by PCI domain number and combine it with the function number to form the
> complete logical device ID for hypercalls.
>
> The prefix construction is shared via hv_build_logical_dev_id_prefix() so
> that pci-hyperv's interrupt retargeting path and the registry use exactly
> the same byte layout. It is derived on demand from the constant hv_device
> instance GUID rather than cached in struct hv_pcibus_device, which is
> private to the pci-hyperv module; this keeps the interface narrow and
> avoids depending on pci-hyperv internals.
>
> Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> Signed-off-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> ---
> drivers/hv/hv_common.c | 95 +++++++++++++++++++++++++++++
> drivers/pci/controller/pci-hyperv.c | 21 +++++--
> include/asm-generic/mshyperv.h | 13 ++++
> include/linux/hyperv.h | 8 +++
> 4 files changed, 132 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 6b67ac616789..53493f8d14dc 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -26,6 +26,8 @@
> #include <linux/kmsg_dump.h>
> #include <linux/sizes.h>
> #include <linux/slab.h>
> +#include <linux/list.h>
> +#include <linux/spinlock.h>
> #include <linux/dma-map-ops.h>
> #include <linux/set_memory.h>
> #include <hyperv/hvhdk.h>
> @@ -863,3 +865,96 @@ const char *hv_result_to_string(u64 status)
> return "Unknown";
> }
> EXPORT_SYMBOL_GPL(hv_result_to_string);
> +
> +#ifdef CONFIG_HYPERV_PVIOMMU
> +/*
> + * Logical device ID registry shared between the vPCI bus driver
> + * (pci-hyperv) and the para-virtualized IOMMU driver. The vPCI driver
> + * registers the per-bus logical device ID prefix at bus probe time, and
> + * the pvIOMMU driver looks it up to build the full logical device ID used
> + * in IOMMU hypercalls.
> + */
> +struct hv_pci_busdata {
> + int pci_domain_nr;
> + u32 logical_dev_id_prefix;
> + struct list_head list;
> +};
> +
> +static LIST_HEAD(hv_pci_bus_list);
> +static DEFINE_SPINLOCK(hv_pci_bus_lock);
> +
> +int hv_iommu_register_pci_bus(int pci_domain_nr, u32 logical_dev_id_prefix)
> +{
> + struct hv_pci_busdata *bus, *new;
> + int ret = 0;
> +
> + new = kzalloc_obj(*new, GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + spin_lock(&hv_pci_bus_lock);
> + list_for_each_entry(bus, &hv_pci_bus_list, list) {
> + if (bus->pci_domain_nr != pci_domain_nr)
> + continue;
> +
> + if (bus->logical_dev_id_prefix != logical_dev_id_prefix) {
> + pr_err("stale registration for PCI domain %d (old prefix 0x%08x, new 0x%08x)\n",
> + pci_domain_nr, bus->logical_dev_id_prefix,
> + logical_dev_id_prefix);
> + ret = -EEXIST;
> + }
> +
> + goto out_free;
> + }
> +
> + new->pci_domain_nr = pci_domain_nr;
> + new->logical_dev_id_prefix = logical_dev_id_prefix;
> + list_add(&new->list, &hv_pci_bus_list);
> + spin_unlock(&hv_pci_bus_lock);
> + return 0;
> +
> +out_free:
> + spin_unlock(&hv_pci_bus_lock);
> + kfree(new);
> + return ret;
> +}
> +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_register_pci_bus, "pci-hyperv");
> +
> +void hv_iommu_unregister_pci_bus(int pci_domain_nr)
> +{
> + struct hv_pci_busdata *bus, *tmp;
> +
> + spin_lock(&hv_pci_bus_lock);
> + list_for_each_entry_safe(bus, tmp, &hv_pci_bus_list, list) {
> + if (bus->pci_domain_nr == pci_domain_nr) {
> + list_del(&bus->list);
> + kfree(bus);
> + break;
> + }
> + }
> + spin_unlock(&hv_pci_bus_lock);
> +}
> +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_unregister_pci_bus, "pci-hyperv");
> +
> +/*
> + * Look up the logical device ID prefix registered for @pci_domain_nr.
> + * Returns 0 on success with *prefix filled in; -ENODEV if no entry is
> + * registered for that PCI domain.
> + */
> +int hv_iommu_lookup_logical_dev_id(int pci_domain_nr, u32 *prefix)
> +{
> + struct hv_pci_busdata *bus;
> + int ret = -ENODEV;
> +
> + spin_lock(&hv_pci_bus_lock);
> + list_for_each_entry(bus, &hv_pci_bus_list, list) {
> + if (bus->pci_domain_nr == pci_domain_nr) {
> + *prefix = bus->logical_dev_id_prefix;
> + ret = 0;
> + break;
> + }
> + }
> + spin_unlock(&hv_pci_bus_lock);
> + return ret;
> +}
I started thinking about the mechanism here because it's somewhat
annoying that it takes 77 lines of code (sans comments) to manage
this simple little mapping. I also started thinking about how many entries
are likely to be in the mapping. A guest VM probably has fewer than 10
entries unless it has multiple NICs and maybe some GPUs. But this code
is also intended to be used by the Linux-as-root code, and I'm thinking
that the number of PCI devices managed by the root could easily be a
hundred or more if the root is managing a couple dozen VMs on a large
physical server. Searching a linked list with 100 or more entries could be
a bit slow.
If only the guest scenario were needed, you could declare a static
array with 64 entries (64 is an arbitrary upper bound), and just search
through the array instead of having to allocate memory, deal with
allocation failures, and deal with linked lists. But a fixed size array
would need to be much bigger for the root scenario, and you would
still be doing a linear search.
A better alternative to consider is rhashtable, which is an existing
Linux kernel facility. Based on what an AI bot generated for me, the
code for setting up and using rhashtable is straightforward, and
would probably result in far fewer than 77 lines of code. Lookups
would also be faster than a linear search, at least for the root case
with more than just a few entries. I'd suggest looking at rhashtable
to see whether you like how the resulting code comes out for this
use case, and whether it really is simpler than a roll-your-own linked
list.
Michael
> +#endif /* CONFIG_HYPERV_PVIOMMU */
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index cfc8fa403dad..58ca2c95bd10 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -641,10 +641,7 @@ static void hv_irq_retarget_interrupt(struct irq_data *data)
> params->int_entry.source = HV_INTERRUPT_SOURCE_MSI;
> params->int_entry.msi_entry.address.as_uint32 = int_desc->address & 0xffffffff;
> params->int_entry.msi_entry.data.as_uint32 = int_desc->data;
> - params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> - (hbus->hdev->dev_instance.b[4] << 16) |
> - (hbus->hdev->dev_instance.b[7] << 8) |
> - (hbus->hdev->dev_instance.b[6] & 0xf8) |
> + params->device_id = hv_build_logical_dev_id_prefix(hbus->hdev) |
> PCI_FUNC(pdev->devfn);
> params->int_target.vector = hv_msi_get_int_vector(data);
>
> @@ -3715,6 +3712,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> struct hv_pcibus_device *hbus;
> int ret, dom;
> u16 dom_req;
> + u32 prefix;
> char *name;
>
> bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> @@ -3857,13 +3855,22 @@ static int hv_pci_probe(struct hv_device *hdev,
>
> hbus->state = hv_pcibus_probed;
>
> - ret = create_root_hv_pci_bus(hbus);
> + /* Notify pvIOMMU before any device on the bus is scanned. */
> + prefix = hv_build_logical_dev_id_prefix(hdev);
> +
> + ret = hv_iommu_register_pci_bus(dom, prefix);
> if (ret)
> goto free_windows;
>
> + ret = create_root_hv_pci_bus(hbus);
> + if (ret)
> + goto unregister_pviommu;
> +
> mutex_unlock(&hbus->state_lock);
> return 0;
>
> +unregister_pviommu:
> + hv_iommu_unregister_pci_bus(dom);
> free_windows:
> hv_pci_free_bridge_windows(hbus);
> exit_d0:
> @@ -3977,6 +3984,8 @@ static void hv_pci_remove(struct hv_device *hdev)
>
> hbus = hv_get_drvdata(hdev);
> if (hbus->state == hv_pcibus_installed) {
> + int dom = hbus->bridge->domain_nr;
> +
> tasklet_disable(&hdev->channel->callback_event);
> hbus->state = hv_pcibus_removing;
> tasklet_enable(&hdev->channel->callback_event);
> @@ -3994,6 +4003,8 @@ static void hv_pci_remove(struct hv_device *hdev)
> hv_pci_remove_slots(hbus);
> pci_remove_root_bus(hbus->bridge->bus);
> pci_unlock_rescan_remove();
> +
> + hv_iommu_unregister_pci_bus(dom);
> }
>
> hv_pci_bus_exit(hdev, false);
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index bf601d67cecb..f65344f2bb81 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -73,6 +73,19 @@ extern enum hv_partition_type hv_curr_partition_type;
> extern void * __percpu *hyperv_pcpu_input_arg;
> extern void * __percpu *hyperv_pcpu_output_arg;
>
> +#ifdef CONFIG_HYPERV_PVIOMMU
> +int hv_iommu_register_pci_bus(int pci_domain_nr, u32 logical_dev_id_prefix);
> +void hv_iommu_unregister_pci_bus(int pci_domain_nr);
> +int hv_iommu_lookup_logical_dev_id(int pci_domain_nr, u32 *prefix);
> +#else
> +static inline int hv_iommu_register_pci_bus(int pci_domain_nr,
> + u32 logical_dev_id_prefix)
> +{
> + return 0;
> +}
> +static inline void hv_iommu_unregister_pci_bus(int pci_domain_nr) { }
> +#endif
> +
> u64 hv_do_hypercall(u64 control, void *inputaddr, void *outputaddr);
> u64 hv_do_fast_hypercall8(u16 control, u64 input8);
> u64 hv_do_fast_hypercall16(u16 control, u64 input1, u64 input2);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 9de2c8d6037a..10ee2c462d7c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1287,6 +1287,14 @@ struct hv_device {
> #define device_to_hv_device(d) container_of_const(d, struct hv_device, device)
> #define drv_to_hv_drv(d) container_of_const(d, struct hv_driver, driver)
>
> +static inline u32 hv_build_logical_dev_id_prefix(struct hv_device *hdev)
> +{
> + return ((u32)hdev->dev_instance.b[5] << 24) |
> + ((u32)hdev->dev_instance.b[4] << 16) |
> + ((u32)hdev->dev_instance.b[7] << 8) |
> + (hdev->dev_instance.b[6] & 0xf8u);
> +}
> +
> static inline void hv_set_drvdata(struct hv_device *dev, void *data)
> {
> dev_set_drvdata(&dev->device, data);
> --
> 2.52.0
>
^ permalink raw reply
* Re: [PATCH v5 08/51] x86/sev: Shove SNP's secure/trusted TSC frequency directly into "calibration"
From: Nikunj A. Dadhania @ 2026-07-08 4:52 UTC (permalink / raw)
To: Sean Christopherson, Jonathan Corbet, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Rick Edgecombe, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Andy Lutomirski, Peter Zijlstra, Juergen Gross,
Daniel Lezcano, John Stultz
Cc: Shuah Khan, H. Peter Anvin, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, linux-doc, kvm, linux-kernel, linux-coco,
linux-hyperv, virtualization, xen-devel, Tom Lendacky,
David Woodhouse, David Woodhouse, Michael Kelley, Thomas Gleixner
In-Reply-To: <20260701193212.749551-9-seanjc@google.com>
On 7/2/2026 1:01 AM, Sean Christopherson wrote:
> As a first step towards dropping .calibrate_{cpu,tsc}() and explicitly
> defining precedence/priority for "calibration" routines, pass the secure
> TSC frequency obtained from SNP firmware directly to
> determine_cpu_tsc_frequencies() instead of overriding the .calibrate_tsc()
> hook.
>
> Unlike the native calibration routines, all of the paravirtual overrides,
> including SNP and TDX, are constant in the sense that the frequency
> provided by the hypervisor or trusted firmware is fixed, known, and always
> available during early boot. More importantly, for CoCo (SNP and TDX) VMs,
> it's imperative that the kernel uses the frequency provided by the trusted
> firmware, not by the untrusted hypervisor. Enforcing the priority between
> sources by carefully ordering seemingly unrelated init calls, so that the
> trusted override "wins", is brittle and all but impossible to follow.
>
> Explicitly ignore tsc_early_khz if the exact TSC frequency was obtained
> from trusted firmware, as per commit bd35c77e32e4 ("x86/tsc: Add
> tsc_early_khz command line parameter"), the goal of the param is to play
> nice with setups that provide partial frequency information in CPUID, i.e.
> is NOT intended to be a hard override. Neither SNP's secure TSC nor TDX
> was supported when commit bd35c77e32e4 landed back in 2020, i.e. lack of
> consideration for the interaction was purely due to oversight when SNP and
> TDX support came along.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> .../admin-guide/kernel-parameters.txt | 4 +++
> arch/x86/coco/sev/core.c | 14 +++--------
> arch/x86/include/asm/sev.h | 4 +--
> arch/x86/kernel/tsc.c | 25 ++++++++++++++-----
> 4 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index b5493a7f8f22..181149f633c3 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -7946,6 +7946,10 @@ Kernel parameters
> with CPUID.16h support and partial CPUID.15h support.
> Format: <unsigned int>
>
> + Note, tsc_early_khz is ignored if the TSC frequency is
> + provided by trusted firmware when running as an SNP
> + guest.
> +
> tsx= [X86] Control Transactional Synchronization
> Extensions (TSX) feature in Intel processors that
> support TSX control.
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 403dcea86452..bc5ae9ef74da 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -99,7 +99,6 @@ static const char * const sev_status_feat_names[] = {
> */
> static u64 snp_tsc_scale __ro_after_init;
> static u64 snp_tsc_offset __ro_after_init;
> -static unsigned long snp_tsc_freq_khz __ro_after_init;
>
> DEFINE_PER_CPU(struct sev_es_runtime_data*, runtime_data);
> DEFINE_PER_CPU(struct sev_es_save_area *, sev_vmsa);
> @@ -2014,15 +2013,10 @@ void __init snp_secure_tsc_prepare(void)
> pr_debug("SecureTSC enabled");
> }
>
> -static unsigned long securetsc_get_tsc_khz(void)
> -{
> - return snp_tsc_freq_khz;
> -}
> -
> -void __init snp_secure_tsc_init(void)
> +unsigned int __init snp_secure_tsc_init(void)
> {
> + unsigned long snp_tsc_freq_khz, tsc_freq_mhz;
> struct snp_secrets_page *secrets;
> - unsigned long tsc_freq_mhz;
> void *mem;
>
> mem = early_memremap_encrypted(sev_secrets_pa, PAGE_SIZE);
> @@ -2043,7 +2037,7 @@ void __init snp_secure_tsc_init(void)
>
> snp_tsc_freq_khz = SNP_SCALE_TSC_FREQ(tsc_freq_mhz * 1000, secrets->tsc_factor);
>
> - x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
> -
> early_memunmap(mem, PAGE_SIZE);
> +
> + return snp_tsc_freq_khz;
> }
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 594cfa19cbd4..05ebf0b73ef4 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -530,7 +530,7 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
> int snp_svsm_vtpm_send_command(u8 *buffer);
>
> void __init snp_secure_tsc_prepare(void);
> -void __init snp_secure_tsc_init(void);
> +unsigned int snp_secure_tsc_init(void);
It seems __init got dropped here accidentally?
Apart from this:
Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
Tested-by: Nikunj A Dadhania <nikunj@amd.com>
> enum es_result savic_register_gpa(u64 gpa);
> enum es_result savic_unregister_gpa(u64 *gpa);
> u64 savic_ghcb_msr_read(u32 reg);
> @@ -637,7 +637,7 @@ static inline int snp_send_guest_request(struct snp_msg_desc *mdesc,
> struct snp_guest_req *req) { return -ENODEV; }
> static inline int snp_svsm_vtpm_send_command(u8 *buffer) { return -ENODEV; }
> static inline void __init snp_secure_tsc_prepare(void) { }
> -static inline void __init snp_secure_tsc_init(void) { }
> +static inline unsigned int __init snp_secure_tsc_init(void) { return 0; }
> static inline void sev_evict_cache(void *va, int npages) {}
> static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED; }
> static inline enum es_result savic_unregister_gpa(u64 *gpa) { return ES_UNSUPPORTED; }
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 8f1604ffe986..f049c126e47c 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1440,15 +1440,16 @@ static int __init init_tsc_clocksource(void)
> */
> device_initcall(init_tsc_clocksource);
>
> -static bool __init determine_cpu_tsc_frequencies(bool early)
> +static bool __init determine_cpu_tsc_frequencies(bool early,
> + unsigned int known_tsc_khz)
> {
> /* Make sure that cpu and tsc are not already calibrated */
> WARN_ON(cpu_khz || tsc_khz);
>
> if (early) {
> cpu_khz = x86_platform.calibrate_cpu();
> - if (tsc_early_khz)
> - tsc_khz = tsc_early_khz;
> + if (known_tsc_khz)
> + tsc_khz = known_tsc_khz;
> else
> tsc_khz = x86_platform.calibrate_tsc();
> } else {
> @@ -1503,6 +1504,8 @@ static void __init tsc_enable_sched_clock(void)
>
> void __init tsc_early_init(void)
> {
> + unsigned int known_tsc_khz = 0;
> +
> if (!boot_cpu_has(X86_FEATURE_TSC))
> return;
> /* Don't change UV TSC multi-chassis synchronization */
> @@ -1510,9 +1513,19 @@ void __init tsc_early_init(void)
> return;
>
> if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC))
> - snp_secure_tsc_init();
> + known_tsc_khz = snp_secure_tsc_init();
>
> - if (!determine_cpu_tsc_frequencies(true))
> + /*
> + * Ignore the user-provided TSC frequency if the exact frequency was
> + * obtained from trusted firmware, as the user-provided frequency is
> + * intended as a "starting point", not a known, guaranteed frequency.
> + */
> + if (!known_tsc_khz)
> + known_tsc_khz = tsc_early_khz;
> + else if (tsc_early_khz)
> + pr_err("Ignoring 'tsc_early_khz' in favor of trusted firmware.\n");
> +
> + if (!determine_cpu_tsc_frequencies(true, known_tsc_khz))
> return;
> tsc_enable_sched_clock();
> }
> @@ -1533,7 +1546,7 @@ void __init tsc_init(void)
>
> if (!tsc_khz) {
> /* We failed to determine frequencies earlier, try again */
> - if (!determine_cpu_tsc_frequencies(false)) {
> + if (!determine_cpu_tsc_frequencies(false, 0)) {
> mark_tsc_unstable("could not calculate TSC khz");
> setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE_TIMER);
> return;
^ permalink raw reply
* Re: [PATCH v5 06/51] x86/sev: Don't override CPU frequency calibration for SNP's Secure TSC
From: Nikunj A. Dadhania @ 2026-07-08 4:54 UTC (permalink / raw)
To: Sean Christopherson, Jonathan Corbet, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Rick Edgecombe, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Andy Lutomirski, Peter Zijlstra, Juergen Gross,
Daniel Lezcano, John Stultz
Cc: Shuah Khan, H. Peter Anvin, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, linux-doc, kvm, linux-kernel, linux-coco,
linux-hyperv, virtualization, xen-devel, Tom Lendacky,
David Woodhouse, David Woodhouse, Michael Kelley, Thomas Gleixner
In-Reply-To: <20260701193212.749551-7-seanjc@google.com>
On 7/2/2026 1:01 AM, Sean Christopherson wrote:
> Don't override the kernel's CPU frequency calibration routine when
> registering SNP's Secure TSC calibration routine. SNP (the architecture)
> provides zero guarantees that the CPU runs at the same frequency as the
> TSC. The justification for clobbering the CPU routine was:
>
> Since the difference between CPU base and TSC frequency does not apply
> in this case, the same callback is being used.
>
> but that's simply not true. E.g. if APERF/MPERF is exposed to the VM, then
> the CPU frequency absolutely does matter.
>
> While relying on heuristics and/or the untrusted hypervisor to provide the
> CPU frequency isn't ideal, it's at least not outright wrong.
>
> Fixes: 73bbf3b0fbba ("x86/tsc: Init the TSC for Secure TSC guests")
> Cc: Nikunj A Dadhania <nikunj@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
> arch/x86/coco/sev/core.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index ed0ac52a765e..665de1aea0ee 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -2046,7 +2046,6 @@ void __init snp_secure_tsc_init(void)
>
> snp_tsc_freq_khz = SNP_SCALE_TSC_FREQ(tsc_freq_mhz * 1000, secrets->tsc_factor);
>
> - x86_platform.calibrate_cpu = securetsc_get_tsc_khz;
> x86_platform.calibrate_tsc = securetsc_get_tsc_khz;
>
> early_memunmap(mem, PAGE_SIZE);
^ permalink raw reply
* Re: [PATCH v5 47/51] x86/paravirt: Don't use a PV sched_clock in CoCo guests with trusted TSC
From: Nikunj A. Dadhania @ 2026-07-08 5:00 UTC (permalink / raw)
To: Sean Christopherson, Jonathan Corbet, Paolo Bonzini,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Rick Edgecombe, K. Y. Srinivasan, Haiyang Zhang,
Wei Liu, Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov,
Jan Kiszka, Andy Lutomirski, Peter Zijlstra, Juergen Gross,
Daniel Lezcano, John Stultz
Cc: Shuah Khan, H. Peter Anvin, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, linux-doc, kvm, linux-kernel, linux-coco,
linux-hyperv, virtualization, xen-devel, Tom Lendacky,
David Woodhouse, David Woodhouse, Michael Kelley, Thomas Gleixner
In-Reply-To: <20260701193212.749551-48-seanjc@google.com>
On 7/2/2026 1:02 AM, Sean Christopherson wrote:
> Silently ignore attempts to switch to a paravirt sched_clock when running
> as a CoCo guest with trusted TSC. In hand-wavy theory, a misbehaving
> hypervisor could attack the guest by manipulating the PV clock to affect
> guest scheduling in some weird and/or predictable way. More importantly,
> reading TSC on such platforms is faster than any PV clock, and sched_clock
> is all about speed.
>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
> arch/x86/kernel/tsc.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index 012321fed5e5..a146fc7b5e74 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -283,6 +283,15 @@ bool using_native_sched_clock(void)
> int __init __paravirt_set_sched_clock(u64 (*func)(void), bool stable,
> void (*save)(void), void (*restore)(void))
> {
> + /*
> + * Don't replace TSC with a PV clock when running as a CoCo guest and
> + * the TSC is secure/trusted; PV clocks are emulated by the hypervisor,
> + * which isn't in the guest's TCB.
> + */
> + if (cc_platform_has(CC_ATTR_GUEST_SNP_SECURE_TSC) ||
> + boot_cpu_has(X86_FEATURE_TDX_GUEST))
> + return -EPERM;
> +
> if (!stable)
> clear_sched_clock_stable();
>
^ permalink raw reply
* Re: [PATCH net-next v2] net: mana: Add handler for sriov configure
From: Paolo Abeni @ 2026-07-08 8:08 UTC (permalink / raw)
To: haiyangz
Cc: linux-hyperv, netdev, kys, haiyangz, wei.liu, decui, longli,
andrew+netdev, davem, edumazet, kuba, pabeni, horms, ernis,
dipayanroy, gargaditya, shradhagupta, linux-kernel, paulros
In-Reply-To: <20260701180116.507690-1-haiyangz@linux.microsoft.com>
From: AI Reviewer <ai@example.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net: mana: Add handler for sriov configure
Add callback function for the pci_driver / sriov_configure.
It asks the NIC to provide certain number of VFs, or disable
VFs if the request is zero.
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index e8b7ffb47eb98..f7a79874fd0eb 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -2496,6 +2496,20 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
> pci_disable_device(pdev);
> }
[Severity: High]
With the addition of SR-IOV support in this patch, do mana_gd_shutdown() and
mana_gd_remove() need to call pci_disable_sriov()?
If a user enables VFs via sysfs and then unbinds the driver or shuts down the
system, could this omission introduce a regression where orphaned, dead VF PCI
devices are left in the kernel?
Accessing these orphaned VFs might trigger use-after-free crashes or hardware
faults, as the underlying PF hardware and driver context have been destroyed.
> +static int mana_sriov_configure(struct pci_dev *pdev, int numvfs)
> +{
> + int err = 0;
> +
> + dev_info(&pdev->dev, "Requested num VFs: %d\n", numvfs);
> +
> + if (numvfs > 0)
> + err = pci_enable_sriov(pdev, numvfs);
> + else
> + pci_disable_sriov(pdev);
[Severity: High]
Should this path check if the VFs are currently assigned to a guest VM before
disabling SR-IOV?
If a privileged user writes 0 to sriov_numvfs while VFs are actively assigned
to a VM (e.g., via vfio-pci), could forcefully removing the PCI devices cause
a regression resulting in host IOMMU faults and guest kernel panics?
Would it be better to check pci_vfs_assigned(pdev) here, or use the
pci_sriov_configure_simple() helper to safely enforce this?
> +
> + return err ? err : numvfs;
> +}
--
This is an AI-generated review.
^ permalink raw reply
* Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag
From: Paolo Abeni @ 2026-07-08 9:35 UTC (permalink / raw)
To: dipayanroy
Cc: kys, haiyangz, wei.liu, decui, andrew+netdev, davem, edumazet,
kuba, pabeni, leon, longli, kotaranov, horms, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov,
pavan.chebbi
In-Reply-To: <20260701141808.461554-3-dipayanroy@linux.microsoft.com>
From: AI Reviewer <ai@example.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag
This adds an ethtool private flag "full-page-rx" that forces one RX buffer
per page to avoid a page_pool fragment path throughput regression on some
4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up
triggers a mana_detach() + mana_attach() reconfiguration to apply the new
buffer layout.
A few questions inline.
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7438ea6b3f26..38f50b4eef87 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
unrelated to the comments below ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 3531abfdaf43..24f6ce63b031 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
[ ... ]
> @@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
> return 0;
> }
>
> +static u32 mana_get_priv_flags(struct net_device *ndev)
> +{
> + struct mana_port_context *apc = netdev_priv(ndev);
> +
> + return apc->priv_flags;
> +}
> +
> +static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
> +{
> + struct mana_port_context *apc = netdev_priv(ndev);
> + u32 changed = apc->priv_flags ^ priv_flags;
> + u32 old_priv_flags = apc->priv_flags;
> + bool schedule_port_reset = false;
> + int err = 0;
> +
> + if (!changed)
> + return 0;
> +
> + /* Reject unknown bits */
> + if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
> + return -EINVAL;
> +
> + if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> + apc->priv_flags = priv_flags;
[Low]
Is the storage of priv_flags intentionally nested inside the per-flag
branch? With only one flag today this is fine, but when a second bit is
added to enum mana_priv_flag_bits, a user toggling only the new bit
passes the !changed and unknown-bits checks, the outer if is skipped,
apc->priv_flags is never updated, and mana_set_priv_flags() returns 0
while mana_get_priv_flags() keeps reporting the old state.
Would it be cleaner to store priv_flags unconditionally after validation
and use per-flag if (changed & BIT(...)) blocks only for the
flag-specific reconfiguration?
> +
> + if (!apc->port_is_up) {
> + /* Port is down, flag updated to apply on next up
> + * so just return.
> + */
> + return 0;
> + }
> +
> + /* Pre-allocate buffers to prevent failure in mana_attach
> + * later
> + */
> + err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or
XDP is attached, mana_use_single_rxbuf_per_page() already returns true
regardless of the new priv flag:
if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
return true;
/* For xdp and jumbo frames make sure only one packet fits per page. */
if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
return true;
In that case mana_get_rxbuf_cfg() produces identical output whether the
new flag is on or off, yet toggling the flag here always performs a full
mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps
the link.
Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth
adding to skip the reconfiguration when nothing actually changes?
[High]
mana_set_priv_flags() drives the same detach/attach sequence used by
mana_set_channels() and mana_per_port_queue_reset_work_handler(), but
does not take apc->vport_mutex and set apc->channel_changing = true
across the window.
The struct comment on channel_changing spells out the invariant:
/* Set by mana_set_channels() under vport_mutex to block RDMA
* from grabbing the vport during the detach/attach window.
* Checked by mana_cfg_vport() when called from the RDMA path.
*/
bool channel_changing;
Without it, after mana_detach() uncfg's the vport an RDMA client can
call mana_cfg_vport() and bump vport_use_count, and then mana_attach()
will fail with -EBUSY when it re-cfg's the vport.
There is also no equivalent of the '!port_is_up && vport_use_count'
early -EBUSY check that mana_set_channels() performs, so is the flag
allowed to be changed silently while an RDMA client is using the vport?
> + if (err) {
> + netdev_err(ndev,
> + "Insufficient memory for new allocations\n");
> + apc->priv_flags = old_priv_flags;
> + return err;
> + }
> +
> + err = mana_detach(ndev, false);
[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach()
called below. Is RTNL guaranteed to be held on the SET priv-flags path?
mana registers net_shaper_ops in mana_devops, which makes it an
ops-locked driver, so netdev_need_ops_lock() is true and the ethtool
core acquires rtnl only when the driver opts in via
ETHTOOL_OP_NEEDS_RTNL_* for that specific command.
Looking at mana_ethtool_ops.op_needs_rtnl:
.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
ETHTOOL_OP_NEEDS_RTNL_GLINK,
ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the
detach/attach helpers below will WARN, and the code races with
RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu,
mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs,
apc->tx_qp and apc->port_st_save.
Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching
what is already done for SET channels?
> + if (err) {
> + netdev_err(ndev, "mana_detach failed: %d\n", err);
> + apc->priv_flags = old_priv_flags;
> +
> + /* Port is in an inconsistent state. Restore
> + * 'port_is_up' so that queue reset work handler
> + * can properly detach and re-attach.
> + */
> + apc->port_is_up = true;
> + schedule_port_reset = true;
> + goto out;
> + }
> +
> + err = mana_attach(ndev);
> + if (err) {
> + netdev_err(ndev, "mana_attach failed: %d\n", err);
> + apc->priv_flags = old_priv_flags;
> +
> + /* Restore 'port_is_up' so the reset work handler
> + * can properly detach/attach. Without this,
> + * the handler sees port_is_up=false and skips
> + * queue allocation, leaving the port dead.
> + */
> + apc->port_is_up = true;
> + schedule_port_reset = true;
[High]
On this branch, mana_detach() has already succeeded and run
mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn
down. Setting apc->port_is_up = true here between now and when
queue_reset_work runs opens a window where callers gate on port_is_up
as their "safe to access rxqs/tx_qp" predicate.
For example, mana_get_ethtool_stats() does:
if (!apc->port_is_up)
return;
...
rxq = apc->rxqs[q];
...
A concurrent ethtool -S invocation during that window will pass the
port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.
Is the port_is_up restore actually needed for the reset work handler's
mana_detach() call? On the reset path, mana_detach() takes its early
return when !netif_device_present(ndev):
if (!from_close && !netif_device_present(ndev))
return 0;
That early return does not touch apc->port_st_save, so the saved state
from the earlier successful mana_detach() should already be intact for
the follow-up mana_attach().
There is also no smp_wmb() paired with this write, unlike the pattern
used inside mana_detach()/mana_attach() proper.
> + }
> + }
> +
> +out:
> + mana_pre_dealloc_rxbufs(apc);
> +
> + if (schedule_port_reset)
> + queue_work(apc->ac->per_port_queue_reset_wq,
> + &apc->queue_reset_work);
> +
> + return err;
> +}
> +
[ ... remaining hunks unrelated to the comments above ... ]
--
This is an AI-generated review.
^ permalink raw reply
* Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Yu Zhang @ 2026-07-08 10:33 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, mhklinux, jacob.pan,
tgopinath, easwar.hariharan, mrathor
In-Reply-To: <20260703173248.GB1968184@ziepe.ca>
On Fri, Jul 03, 2026 at 02:32:48PM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 03, 2026 at 12:05:17AM +0800, Yu Zhang wrote:
>
> > +static bool hv_iommu_capable(struct device *dev, enum iommu_cap cap)
> > +{
> > + switch (cap) {
> > + case IOMMU_CAP_CACHE_COHERENCY:
> > + return true;
> > + case IOMMU_CAP_DEFERRED_FLUSH:
> > + return true;
>
> This CAP isn't necessary anymore
>
Right, thanks for pointing this out!
> > +static struct iommu_device *hv_iommu_probe_device(struct device *dev)
> > +{
> > + struct pci_dev *pdev;
> > + struct hv_iommu_endpoint *vdev;
> > + struct hv_output_get_logical_device_property device_iommu_property = {0};
> > +
> > + if (!dev_is_pci(dev))
> > + return ERR_PTR(-ENODEV);
> > +
> > + pdev = to_pci_dev(dev);
> > +
> > + if (hv_iommu_get_logical_device_property(dev,
> > + HV_LOGICAL_DEVICE_PROPERTY_PVIOMMU,
> > + &device_iommu_property) ||
> > + !(device_iommu_property.device_iommu & HV_DEVICE_IOMMU_ENABLED))
> > + return ERR_PTR(-ENODEV);
> > +
> > + vdev = kzalloc_obj(*vdev, GFP_KERNEL);
> > + if (!vdev)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + vdev->dev = dev;
> > + vdev->hv_iommu = hv_iommu_device;
> > + dev_iommu_priv_set(dev, vdev);
> > +
> > + if (hv_iommu_ats_supported(hv_iommu_device->cap) &&
> > + pci_ats_supported(pdev))
> > + pci_enable_ats(pdev, __ffs(hv_iommu_device->pgsize_bitmap));
>
> This can probably just be PAGE_SHIFT
Indeed.
>
> Also ATS shouldn't be enabled until a translation is installed,
> otherwise the driver cannot participate in the ATS error handling
> Nicolin is working on.
>
Yes.I'll move ATS enablement into a paging-domain-specific attach wrapper
that calls pci_enable_ats(pdev, PAGE_SHIFT) after the attach hypercall
succeeds. And maybe blocking attach shall also call pci_disable_ats()
before the hypercall to prevent stale ATC entries from bypassing the
block?
Something like:
static int hv_iommu_paging_attach_dev(...)
{
ret = hv_iommu_attach_dev(domain, dev, old);
if (ret)
return ret;
if (!pdev->ats_enabled && ats_supported)
pci_enable_ats(pdev, PAGE_SHIFT);
return 0;
}
static int hv_iommu_blocking_attach_dev(...)
{
if (pdev->ats_enabled)
pci_disable_ats(pdev);
ret = hv_iommu_attach_dev(domain, dev, old);
...
}
Does this look right?
> > +static void hv_iommu_release_device(struct device *dev)
> > +{
> > + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > +
> > + if (pdev->ats_enabled)
> > + pci_disable_ats(pdev);
> > +
> > + dev_iommu_priv_set(dev, NULL);
>
> No necessary, the caller does it
>
Yes. Thanks!
> > +static struct iommu_group *hv_iommu_device_group(struct device *dev)
> > +{
> > + if (dev_is_pci(dev))
> > + return pci_device_group(dev);
> > +
> > + WARN_ON_ONCE(1);
> > + return generic_device_group(dev);
>
> I think you can just return failure here instead of WARN_ON ?
>
Yes, will change to return ERR_PTR(-ENODEV).
> > +static int __init hv_initialize_static_domains(void)
> > +{
> > + int ret;
> > + struct hv_iommu_domain *hv_domain;
> > +
> > + /* Default stage-1 identity domain */
> > + hv_domain = &hv_identity_domain;
> > +
> > + ret = hv_create_device_domain(hv_domain, HV_DEVICE_DOMAIN_TYPE_S1);
> > + if (ret)
> > + return ret;
> > +
> > + ret = hv_configure_device_domain(hv_domain, IOMMU_DOMAIN_IDENTITY);
> > + if (ret)
> > + goto delete_identity_domain;
>
> IMHO I would change this around to have a single function that accepts
> a struct hv_input_configure_device_domain as input and does both of
> the hypercalls inside. Then here it is easy to directly construct the
> hv_input_configure_device_domain for blocking and identity.
>
> I'd be happy if this never touched domain_type, drivers shouldn't be
> touching that.
>
Good idea. Maybe we can just change hv_configure_device_domain()
to take a "struct hv_device_domain_settings *" directly - that way each
caller constructs the HW settings explicitly and domain_type is not
needed at all. Does that sound right?
> > +static void __init hv_init_iommu_device(struct hv_iommu_dev *hv_iommu,
> > + struct hv_output_get_iommu_capabilities *hv_iommu_cap)
> > +{
> > + ida_init(&hv_iommu->domain_ids);
> > +
> > + hv_iommu->cap = hv_iommu_cap->iommu_cap;
> > + hv_iommu->max_iova_width = hv_iommu_cap->max_iova_width;
> > + if (!hv_iommu_5lvl_supported(hv_iommu->cap) &&
> > + hv_iommu->max_iova_width > 48) {
> > + pr_info("5-level paging not supported, limiting iova width to 48.\n");
> > + hv_iommu->max_iova_width = 48;
> > + }
> > +
> > + hv_iommu->geometry = (struct iommu_domain_geometry) {
> > + .aperture_start = 0,
> > + .aperture_end = (((u64)1) << hv_iommu->max_iova_width) - 1,
> > + .force_aperture = true,
> > + };
>
> I don't see anything reading this, I don't expect this to be used?
>
> The max_iova_width has to be passed into the iommupt creation, which
> it does:
>
> + cfg.common.hw_max_vasz_lg2 = hv_iommu_device->max_iova_width;
> + cfg.common.hw_max_oasz_lg2 = 52;
> + cfg.top_level = (hv_iommu_device->max_iova_width > 48) ? 4 : 3;
> + ret = pt_iommu_x86_64_init(&hv_domain->pt_iommu_x86_64, &cfg, GFP_KERNEL);
> + if (ret)
>
> So just delete hv->iommu->geometry.
>
Right. Will remove.
> Also, VT-D has weirdness where the HW can require a 4 level table but
> only a 3 level worth of IOVA width is being used. This was a
> real-world bug we hit when converting to iommupt. This interaction
> with the HV doesn't seem able to represent that.
>
Is this the issue fixed by commit d856f9d27885 ("iommupt/vtd: Allow
VT-d to have a larger table top than the vasz requires")? For pvIOMMU
the first-stage table is either 4-level (max_iova_width <= 48) or
5-level (max_iova_width > 48 && 5lvl cap set). Is there a scenario
where this would still be a problem?
> > + /*
> > + * The page table code only maps x86 page sizes (4K/2M/1G); require the
> > + * hypervisor to advertise a non-empty subset of exactly those.
> > + */
> > + if (!hv_iommu_cap.pgsize_bitmap ||
> > + (hv_iommu_cap.pgsize_bitmap & ~(u64)(SZ_4K | SZ_2M | SZ_1G))) {
> > + pr_err("unsupported page sizes: pgsize_bitmap=0x%llx\n",
> > + hv_iommu_cap.pgsize_bitmap);
> > + return -ENODEV;
> > + }
>
> This can just be
>
> if (!(hv_iommu_cap.pgsize_bitmap & PAGE_SHIFT)) {
> pr_err("unsupported page sizes: pgsize_bitmap=0x%llx\n",
> hv_iommu_cap.pgsize_bitmap);
> } return -ENODEV;
>
> Which is all you really need. If the HV doesn't support 1G it is
> perfectly fine, the iommupt page bitmap is already masked by this.
>
Good point, it's much simpler.
And I assume you meant PAGE_SIZE / SZ_4K instead of PAGE_SHIFT here. :)
> > + ret = iommu_device_register(&hv_iommu->iommu, &hv_iommu_ops, NULL);
> > + if (ret) {
> > + pr_err("iommu_device_register failed: %d\n", ret);
> > + goto err_sysfs_remove;
> > + }
> > +
> > + pr_info("successfully initialized\n");
>
> Don't log someting so vauge?
>
With pr_fmt defined as "Hyper-V pvIOMMU: ", this shows up as
"Hyper-V pvIOMMU: successfully initialized" in dmesg. I'd like to
keep some indication that pvIOMMU init succeeded at boot. Is this
still too vague? Would it be better if I also print capabilities
like IOVA width and supported page sizes here? Thanks!
B.R.
Yu
> Jason
>
^ permalink raw reply
* Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Yu Zhang @ 2026-07-08 10:55 UTC (permalink / raw)
To: Jacob Pan
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, jgg, mhklinux,
tgopinath, easwar.hariharan, mrathor
In-Reply-To: <20260706095510.00007ce1@linux.microsoft.com>
On Mon, Jul 06, 2026 at 09:55:10AM -0700, Jacob Pan wrote:
> Hi Yu,
>
> On Fri, 3 Jul 2026 00:05:17 +0800
> Yu Zhang <zhangyu1@linux.microsoft.com> wrote:
>
> > +static int hv_iommu_attach_dev(struct iommu_domain *domain, struct
> > device *dev,
> > + struct iommu_domain *old)
> > +{
> > + u64 status;
> > + u32 prefix;
> > + unsigned long flags;
> > + struct pci_dev *pdev;
> > + struct hv_input_attach_device_domain *input;
> > + struct hv_iommu_endpoint *vdev = dev_iommu_priv_get(dev);
> > + struct hv_iommu_domain *hv_domain =
> > to_hv_iommu_domain(domain);
> > + int ret;
> > +
> > + if (vdev->hv_domain == hv_domain)
> > + return 0;
> > +
> Is this needed? seems the core code already skips same domain attach?
> i.e.
>
> static int __iommu_group_set_domain_internal(struct iommu_group *group,
> struct iommu_domain
> *new_domain, unsigned int flags)
> {
> ...
> if (group->domain == new_domain)
> return 0;
>
Thanks for catching this, Jacob. Will remove
B.R.
Yu
^ permalink raw reply
* Re: [PATCH net v3 1/2] net: mana: Validate the packet length reported by the NIC
From: Paolo Abeni @ 2026-07-08 10:57 UTC (permalink / raw)
To: Dexuan Cui, kys, haiyangz, wei.liu, longli, andrew+netdev, davem,
edumazet, kuba, kotaranov, horms, ernis, dipayanroy, kees,
jacob.e.keller, ssengar, linux-hyperv, netdev, linux-kernel,
linux-rdma
Cc: stable
In-Reply-To: <20260702041237.617719-2-decui@microsoft.com>
On 7/2/26 6:12 AM, Dexuan Cui wrote:
> Validate the packet length reported in the RX CQE before passing it
> to skb processing. The CQE is supplied by the NIC device and should
> not be blindly trusted.
>
> Cc: stable@vger.kernel.org
This need a Fixes: tag, to help stable team backport.
No need to repost: just reply here, and I'll add it while applying the
patch.
Thanks,
Paolo
^ permalink raw reply
* Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Yu Zhang @ 2026-07-08 11:05 UTC (permalink / raw)
To: Jacob Pan
Cc: Mukesh R, linux-kernel, linux-hyperv, iommu, linux-pci,
linux-arch, wei.liu, kys, haiyangz, decui, longli, joro, will,
robin.murphy, bhelgaas, kwilczynski, lpieralisi, mani, robh, arnd,
jgg, mhklinux, tgopinath, easwar.hariharan
In-Reply-To: <20260707074814.000010a0@linux.microsoft.com>
On Tue, Jul 07, 2026 at 07:48:14AM -0700, Jacob Pan wrote:
> Hi Mukesh,
>
> On Mon, 6 Jul 2026 17:08:50 -0700
> Mukesh R <mrathor@linux.microsoft.com> wrote:
>
> > From: Mukesh R <mrathor@linux.microsoft.com>
> > To: Yu Zhang <zhangyu1@linux.microsoft.com>,
> > linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
> > iommu@lists.linux.dev, linux-pci@vger.kernel.org,
> > linux-arch@vger.kernel.org Cc: wei.liu@kernel.org, kys@microsoft.com,
> > haiyangz@microsoft.com, decui@microsoft.com, longli@microsoft.com,
> > joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
> > bhelgaas@google.com, kwilczynski@kernel.org, lpieralisi@kernel.org,
> > mani@kernel.org, robh@kernel.org, arnd@arndb.de, jgg@ziepe.ca,
> > mhklinux@outlook.com, jacob.pan@linux.microsoft.com,
> > tgopinath@linux.microsoft.com, easwar.hariharan@linux.microsoft.com
> > Subject: Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU
> > support for Hyper-V guest Date: Mon, 6 Jul 2026 17:08:50 -0700
> > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
> > Thunderbird/91.13.1
> >
> > On 7/2/26 09:05, Yu Zhang wrote:
> > > Add a para-virtualized IOMMU driver for Linux guests running on
> > > Hyper-V. This driver implements stage-1 IO translation within the
> > > guest OS. It integrates with the Linux IOMMU core, utilizing
> > > Hyper-V hypercalls for:
> > > - Capability discovery
> > > - Domain allocation, configuration, and deallocation
> > > - Device attachment and detachment
> > > - IOTLB invalidation
> > >
> > > The driver constructs x86-compatible stage-1 IO page tables in the
> > > guest memory using consolidated IO page table helpers. This allows
> > > the guest to manage stage-1 translations independently of vendor-
> > > specific drivers (like Intel VT-d or AMD IOMMU).
> > >
> > > Hyper-V consumes this stage-1 IO page table when a device domain is
> > > created and configured, and nests it with the host's stage-2 IO page
> > > tables, therefore eliminating the VM exits for guest IOMMU mapping
> > > operations. For unmapping operations, VM exits to perform the IOTLB
> > > flush are still unavoidable.
> > >
> > > To identify a device in its hypercall interface, the driver looks
> > > up the logical device ID prefix registered for the device's PCI
> > > domain (see the logical device ID registry in hv_common.c) and
> > > combines it with the PCI function number of the endpoint device.
> > >
> > > Co-developed-by: Wei Liu <wei.liu@kernel.org>
> > > Signed-off-by: Wei Liu <wei.liu@kernel.org>
> > > Co-developed-by: Easwar Hariharan
> > > <easwar.hariharan@linux.microsoft.com> Signed-off-by: Easwar
> > > Hariharan <easwar.hariharan@linux.microsoft.com> Signed-off-by: Yu
> > > Zhang <zhangyu1@linux.microsoft.com> ---
> > > arch/x86/hyperv/hv_init.c | 4 +
> > > arch/x86/include/asm/mshyperv.h | 4 +
> > > drivers/iommu/Kconfig | 1 +
> > > drivers/iommu/hyperv/Kconfig | 16 +
> > > drivers/iommu/hyperv/Makefile | 1 +
> > > drivers/iommu/hyperv/iommu.c | 620
> > > ++++++++++++++++++++++++++++++++ drivers/iommu/hyperv/iommu.h |
> > > 51 +++ 7 files changed, 697 insertions(+)
> > > create mode 100644 drivers/iommu/hyperv/Kconfig
> > > create mode 100644 drivers/iommu/hyperv/iommu.c
> >
> > Hey Jacob,
> >
> > You had suggested I rename iommu.c to iommu-root.c (I called it
> > hv-iommu-root.c eventually), so this needs to be renamed also,
> > right?
>
> yes, I agree. I feel it is clearer to name it hv-iommu-guest.c since
> this is a guest only driver.
>
hv-iommu-guest.c sounds good to me. :)
B.R.
Yu
^ permalink raw reply
* Re: [PATCH v2 3/4] iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest
From: Yu Zhang @ 2026-07-08 11:12 UTC (permalink / raw)
To: Mukesh R
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, jgg, mhklinux,
jacob.pan, tgopinath, easwar.hariharan
In-Reply-To: <8e688b47-eec6-39eb-e4d4-91700edc2e3b@linux.microsoft.com>
On Tue, Jul 07, 2026 at 02:17:02PM -0700, Mukesh R wrote:
[...]
> > +static int hv_create_device_domain(struct hv_iommu_domain *hv_domain, u32 domain_stage)
>
> 99.99% of our code is80 cols, lets keep that way. someday we can change
> all in one shot, until then please avoid line wraps here and below in
> ida_free(), hv_iommu_get_logical_device_property, etc...
>
Sure, thanks for pointing this out.
B.R.
Yu
> Thanks,
> -Mukesh
>
>
^ permalink raw reply
* Re: [PATCH v2 4/4] iommu/hyperv: Add page-selective IOTLB flush support
From: Yu Zhang @ 2026-07-08 12:43 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel, linux-hyperv, iommu, linux-pci, linux-arch, wei.liu,
kys, haiyangz, decui, longli, joro, will, robin.murphy, bhelgaas,
kwilczynski, lpieralisi, mani, robh, arnd, mhklinux, jacob.pan,
tgopinath, easwar.hariharan, mrathor
In-Reply-To: <20260703171018.GA1968184@ziepe.ca>
On Fri, Jul 03, 2026 at 02:10:18PM -0300, Jason Gunthorpe wrote:
> On Fri, Jul 03, 2026 at 12:05:18AM +0800, Yu Zhang wrote:
>
> > @@ -401,10 +402,74 @@ static void hv_iommu_flush_iotlb_all(struct iommu_domain *domain)
> > hv_flush_device_domain(to_hv_iommu_domain(domain));
> > }
> >
> > +/*
> > + * Calculate the minimal power-of-two aligned range that covers [start, end]
> > + * (end is inclusive). Returns a single (page_number, page_mask_shift)
> > + * descriptor that may over-flush when the range is not naturally aligned.
> > + */
> > +static void hv_iommu_calc_flush_range(unsigned long start, unsigned long end,
> > + union hv_iommu_flush_va *va)
> > +{
> > + unsigned long start_pfn = HVPFN_DOWN(start);
> > + unsigned long last_pfn = HVPFN_UP(end + 1) - 1;
>
> Pedantically end can be ULONG_MAX, you shouldn't be adding to it since
> it will overflow.
>
Good catch!
> > + unsigned long mask_shift, aligned_pfn;
> > +
> > + if (start_pfn == last_pfn) {
> > + mask_shift = 0;
> > + } else {
> > + /*
> > + * Find the highest bit position where start_pfn and last_pfn
> > + * differ. A range aligned to one above that bit is the
> > + * smallest power-of-two region that covers both endpoints.
> > + */
> > + mask_shift = __fls(start_pfn ^ last_pfn) + 1;
> > + }
> > +
> > + aligned_pfn = ALIGN_DOWN(start_pfn, 1UL << mask_shift);
>
> I think the whole thing is simpler if it stays using bytes until the end:
>
> sz_lg2 = __fls(gather->start ^ gather->end);
> if (sz_lg2 < HV_PAGE_SHIFT)
> cmd.sz_lg2 = HV_PAGE_SHIFT;
>
> page_number = (gather->start & ~(1UL << sz_lg2)) >> HV_PAGE_SHIFT;
> page_mask_shift = sz_lg2 - HV_PAGE_SIFT;
>
> No overflows that way either
>
Indeed. It is much cleaner. Will rewrite using this logic.
B.R.
Yu
> Jason
>
^ permalink raw reply
* Re: [PATCH] mshv: fix fd leak in mshv_ioctl_create_vtl()
From: Hamza Mahfooz @ 2026-07-08 13:05 UTC (permalink / raw)
To: Yi Xie; +Cc: kys, haiyangz, wei.liu, linux-hyperv, linux-kernel
In-Reply-To: <20260708012852.36824-1-xieyi@kylinos.cn>
On Wed, Jul 08, 2026 at 09:28:52AM +0800, Yi Xie wrote:
> put_unused_fd() if anon_inode_getfile() fails.
>
> Signed-off-by: Yi Xie <xieyi@kylinos.cn>
Reviewed-by: Hamza Mahfooz <hamzamahfooz@linux.microsoft.com>
> ---
> drivers/hv/mshv_vtl_main.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hv/mshv_vtl_main.c b/drivers/hv/mshv_vtl_main.c
> index 0d3d4161974f..897a41b08d02 100644
> --- a/drivers/hv/mshv_vtl_main.c
> +++ b/drivers/hv/mshv_vtl_main.c
> @@ -129,6 +129,7 @@ mshv_ioctl_create_vtl(void __user *user_arg, struct device *module_dev)
> file = anon_inode_getfile("mshv_vtl", &mshv_vtl_fops,
> vtl, O_RDWR);
> if (IS_ERR(file)) {
> + put_unused_fd(fd);
> kfree(vtl);
> return PTR_ERR(file);
> }
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH v2 2/4] Drivers: hv: Add logical device ID registry for vPCI devices
From: Yu Zhang @ 2026-07-08 13:08 UTC (permalink / raw)
To: Michael Kelley
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
iommu@lists.linux.dev, linux-pci@vger.kernel.org,
linux-arch@vger.kernel.org, wei.liu@kernel.org, kys@microsoft.com,
haiyangz@microsoft.com, decui@microsoft.com, longli@microsoft.com,
joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
bhelgaas@google.com, kwilczynski@kernel.org,
lpieralisi@kernel.org, mani@kernel.org, robh@kernel.org,
arnd@arndb.de, jgg@ziepe.ca, jacob.pan@linux.microsoft.com,
tgopinath@linux.microsoft.com,
easwar.hariharan@linux.microsoft.com, mrathor@linux.microsoft.com
In-Reply-To: <SN6PR02MB415798534DFCC14E5202D021D4FF2@SN6PR02MB4157.namprd02.prod.outlook.com>
On Wed, Jul 08, 2026 at 02:52:43AM +0000, Michael Kelley wrote:
> From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Thursday, July 2, 2026 9:05 AM
> >
> > From: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> >
> > Hyper-V identifies each PCI pass-thru device by a logical device ID in
> > its hypercall interface. This ID consists of a per-bus prefix, derived
> > from the VMBus device instance GUID, combined with the PCI function
> > number of the endpoint device.
> >
> > Add a small registry in hv_common.c that maps a PCI domain number to its
> > logical device ID prefix. The vPCI bus driver (pci-hyperv) registers the
> > prefix when a bus is probed and unregisters it when the bus is removed.
> > Consumers such as the para-virtualized IOMMU driver look up the prefix
> > by PCI domain number and combine it with the function number to form the
> > complete logical device ID for hypercalls.
> >
> > The prefix construction is shared via hv_build_logical_dev_id_prefix() so
> > that pci-hyperv's interrupt retargeting path and the registry use exactly
> > the same byte layout. It is derived on demand from the constant hv_device
> > instance GUID rather than cached in struct hv_pcibus_device, which is
> > private to the pci-hyperv module; this keeps the interface narrow and
> > avoids depending on pci-hyperv internals.
> >
> > Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> > Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> > Signed-off-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> > ---
> > drivers/hv/hv_common.c | 95 +++++++++++++++++++++++++++++
> > drivers/pci/controller/pci-hyperv.c | 21 +++++--
> > include/asm-generic/mshyperv.h | 13 ++++
> > include/linux/hyperv.h | 8 +++
> > 4 files changed, 132 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > index 6b67ac616789..53493f8d14dc 100644
> > --- a/drivers/hv/hv_common.c
> > +++ b/drivers/hv/hv_common.c
> > @@ -26,6 +26,8 @@
> > #include <linux/kmsg_dump.h>
> > #include <linux/sizes.h>
> > #include <linux/slab.h>
> > +#include <linux/list.h>
> > +#include <linux/spinlock.h>
> > #include <linux/dma-map-ops.h>
> > #include <linux/set_memory.h>
> > #include <hyperv/hvhdk.h>
> > @@ -863,3 +865,96 @@ const char *hv_result_to_string(u64 status)
> > return "Unknown";
> > }
> > EXPORT_SYMBOL_GPL(hv_result_to_string);
> > +
> > +#ifdef CONFIG_HYPERV_PVIOMMU
> > +/*
> > + * Logical device ID registry shared between the vPCI bus driver
> > + * (pci-hyperv) and the para-virtualized IOMMU driver. The vPCI driver
> > + * registers the per-bus logical device ID prefix at bus probe time, and
> > + * the pvIOMMU driver looks it up to build the full logical device ID used
> > + * in IOMMU hypercalls.
> > + */
> > +struct hv_pci_busdata {
> > + int pci_domain_nr;
> > + u32 logical_dev_id_prefix;
> > + struct list_head list;
> > +};
> > +
> > +static LIST_HEAD(hv_pci_bus_list);
> > +static DEFINE_SPINLOCK(hv_pci_bus_lock);
> > +
> > +int hv_iommu_register_pci_bus(int pci_domain_nr, u32 logical_dev_id_prefix)
> > +{
> > + struct hv_pci_busdata *bus, *new;
> > + int ret = 0;
> > +
> > + new = kzalloc_obj(*new, GFP_KERNEL);
> > + if (!new)
> > + return -ENOMEM;
> > +
> > + spin_lock(&hv_pci_bus_lock);
> > + list_for_each_entry(bus, &hv_pci_bus_list, list) {
> > + if (bus->pci_domain_nr != pci_domain_nr)
> > + continue;
> > +
> > + if (bus->logical_dev_id_prefix != logical_dev_id_prefix) {
> > + pr_err("stale registration for PCI domain %d (old prefix 0x%08x, new 0x%08x)\n",
> > + pci_domain_nr, bus->logical_dev_id_prefix,
> > + logical_dev_id_prefix);
> > + ret = -EEXIST;
> > + }
> > +
> > + goto out_free;
> > + }
> > +
> > + new->pci_domain_nr = pci_domain_nr;
> > + new->logical_dev_id_prefix = logical_dev_id_prefix;
> > + list_add(&new->list, &hv_pci_bus_list);
> > + spin_unlock(&hv_pci_bus_lock);
> > + return 0;
> > +
> > +out_free:
> > + spin_unlock(&hv_pci_bus_lock);
> > + kfree(new);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_register_pci_bus, "pci-hyperv");
> > +
> > +void hv_iommu_unregister_pci_bus(int pci_domain_nr)
> > +{
> > + struct hv_pci_busdata *bus, *tmp;
> > +
> > + spin_lock(&hv_pci_bus_lock);
> > + list_for_each_entry_safe(bus, tmp, &hv_pci_bus_list, list) {
> > + if (bus->pci_domain_nr == pci_domain_nr) {
> > + list_del(&bus->list);
> > + kfree(bus);
> > + break;
> > + }
> > + }
> > + spin_unlock(&hv_pci_bus_lock);
> > +}
> > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_unregister_pci_bus, "pci-hyperv");
> > +
> > +/*
> > + * Look up the logical device ID prefix registered for @pci_domain_nr.
> > + * Returns 0 on success with *prefix filled in; -ENODEV if no entry is
> > + * registered for that PCI domain.
> > + */
> > +int hv_iommu_lookup_logical_dev_id(int pci_domain_nr, u32 *prefix)
> > +{
> > + struct hv_pci_busdata *bus;
> > + int ret = -ENODEV;
> > +
> > + spin_lock(&hv_pci_bus_lock);
> > + list_for_each_entry(bus, &hv_pci_bus_list, list) {
> > + if (bus->pci_domain_nr == pci_domain_nr) {
> > + *prefix = bus->logical_dev_id_prefix;
> > + ret = 0;
> > + break;
> > + }
> > + }
> > + spin_unlock(&hv_pci_bus_lock);
> > + return ret;
> > +}
>
> I started thinking about the mechanism here because it's somewhat
> annoying that it takes 77 lines of code (sans comments) to manage
> this simple little mapping. I also started thinking about how many entries
> are likely to be in the mapping. A guest VM probably has fewer than 10
> entries unless it has multiple NICs and maybe some GPUs. But this code
> is also intended to be used by the Linux-as-root code, and I'm thinking
> that the number of PCI devices managed by the root could easily be a
> hundred or more if the root is managing a couple dozen VMs on a large
> physical server. Searching a linked list with 100 or more entries could be
> a bit slow.
>
> If only the guest scenario were needed, you could declare a static
> array with 64 entries (64 is an arbitrary upper bound), and just search
> through the array instead of having to allocate memory, deal with
> allocation failures, and deal with linked lists. But a fixed size array
> would need to be much bigger for the root scenario, and you would
> still be doing a linear search.
>
> A better alternative to consider is rhashtable, which is an existing
> Linux kernel facility. Based on what an AI bot generated for me, the
> code for setting up and using rhashtable is straightforward, and
> would probably result in far fewer than 77 lines of code. Lookups
> would also be faster than a linear search, at least for the root case
> with more than just a few entries. I'd suggest looking at rhashtable
> to see whether you like how the resulting code comes out for this
> use case, and whether it really is simpler than a roll-your-own linked
> list.
>
Thank you so much for thinking this through, Michael! That is really
a valid concern.
How about using XArray? It might be more lightweight compared with
rhashtable. Using pci_domain_nr as the key and prefix as the value.
Maybe sth. like:
┃
static DEFINE_XARRAY(hv_pci_bus_xa);
int hv_iommu_register_pci_bus(int domain_nr, u32 prefix)
{
return xa_insert(&hv_pci_bus_xa, domain_nr,
xa_mk_value(prefix), GFP_KERNEL);
} ┃
void hv_iommu_unregister_pci_bus(int domain_nr)
{
xa_erase(&hv_pci_bus_xa, domain_nr);
}
int hv_iommu_lookup_logical_dev_id(int domain_nr, u32 *prefix)
{
void *entry = xa_load(&hv_pci_bus_xa, domain_nr);
if (!entry) ┃
return -ENODEV;
*prefix = xa_to_value(entry);
return 0; ┃
}
B.R.
Yu
┃
> Michael
>
> > +#endif /* CONFIG_HYPERV_PVIOMMU */
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index cfc8fa403dad..58ca2c95bd10 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -641,10 +641,7 @@ static void hv_irq_retarget_interrupt(struct irq_data *data)
> > params->int_entry.source = HV_INTERRUPT_SOURCE_MSI;
> > params->int_entry.msi_entry.address.as_uint32 = int_desc->address & 0xffffffff;
> > params->int_entry.msi_entry.data.as_uint32 = int_desc->data;
> > - params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > - (hbus->hdev->dev_instance.b[4] << 16) |
> > - (hbus->hdev->dev_instance.b[7] << 8) |
> > - (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > + params->device_id = hv_build_logical_dev_id_prefix(hbus->hdev) |
> > PCI_FUNC(pdev->devfn);
> > params->int_target.vector = hv_msi_get_int_vector(data);
> >
> > @@ -3715,6 +3712,7 @@ static int hv_pci_probe(struct hv_device *hdev,
> > struct hv_pcibus_device *hbus;
> > int ret, dom;
> > u16 dom_req;
> > + u32 prefix;
> > char *name;
> >
> > bridge = devm_pci_alloc_host_bridge(&hdev->device, 0);
> > @@ -3857,13 +3855,22 @@ static int hv_pci_probe(struct hv_device *hdev,
> >
> > hbus->state = hv_pcibus_probed;
> >
> > - ret = create_root_hv_pci_bus(hbus);
> > + /* Notify pvIOMMU before any device on the bus is scanned. */
> > + prefix = hv_build_logical_dev_id_prefix(hdev);
> > +
> > + ret = hv_iommu_register_pci_bus(dom, prefix);
> > if (ret)
> > goto free_windows;
> >
> > + ret = create_root_hv_pci_bus(hbus);
> > + if (ret)
> > + goto unregister_pviommu;
> > +
> > mutex_unlock(&hbus->state_lock);
> > return 0;
> >
> > +unregister_pviommu:
> > + hv_iommu_unregister_pci_bus(dom);
> > free_windows:
> > hv_pci_free_bridge_windows(hbus);
> > exit_d0:
> > @@ -3977,6 +3984,8 @@ static void hv_pci_remove(struct hv_device *hdev)
> >
> > hbus = hv_get_drvdata(hdev);
> > if (hbus->state == hv_pcibus_installed) {
> > + int dom = hbus->bridge->domain_nr;
> > +
> > tasklet_disable(&hdev->channel->callback_event);
> > hbus->state = hv_pcibus_removing;
> > tasklet_enable(&hdev->channel->callback_event);
> > @@ -3994,6 +4003,8 @@ static void hv_pci_remove(struct hv_device *hdev)
> > hv_pci_remove_slots(hbus);
> > pci_remove_root_bus(hbus->bridge->bus);
> > pci_unlock_rescan_remove();
> > +
> > + hv_iommu_unregister_pci_bus(dom);
> > }
> >
> > hv_pci_bus_exit(hdev, false);
> > diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> > index bf601d67cecb..f65344f2bb81 100644
> > --- a/include/asm-generic/mshyperv.h
> > +++ b/include/asm-generic/mshyperv.h
> > @@ -73,6 +73,19 @@ extern enum hv_partition_type hv_curr_partition_type;
> > extern void * __percpu *hyperv_pcpu_input_arg;
> > extern void * __percpu *hyperv_pcpu_output_arg;
> >
> > +#ifdef CONFIG_HYPERV_PVIOMMU
> > +int hv_iommu_register_pci_bus(int pci_domain_nr, u32 logical_dev_id_prefix);
> > +void hv_iommu_unregister_pci_bus(int pci_domain_nr);
> > +int hv_iommu_lookup_logical_dev_id(int pci_domain_nr, u32 *prefix);
> > +#else
> > +static inline int hv_iommu_register_pci_bus(int pci_domain_nr,
> > + u32 logical_dev_id_prefix)
> > +{
> > + return 0;
> > +}
> > +static inline void hv_iommu_unregister_pci_bus(int pci_domain_nr) { }
> > +#endif
> > +
> > u64 hv_do_hypercall(u64 control, void *inputaddr, void *outputaddr);
> > u64 hv_do_fast_hypercall8(u16 control, u64 input8);
> > u64 hv_do_fast_hypercall16(u16 control, u64 input1, u64 input2);
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 9de2c8d6037a..10ee2c462d7c 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -1287,6 +1287,14 @@ struct hv_device {
> > #define device_to_hv_device(d) container_of_const(d, struct hv_device, device)
> > #define drv_to_hv_drv(d) container_of_const(d, struct hv_driver, driver)
> >
> > +static inline u32 hv_build_logical_dev_id_prefix(struct hv_device *hdev)
> > +{
> > + return ((u32)hdev->dev_instance.b[5] << 24) |
> > + ((u32)hdev->dev_instance.b[4] << 16) |
> > + ((u32)hdev->dev_instance.b[7] << 8) |
> > + (hdev->dev_instance.b[6] & 0xf8u);
> > +}
> > +
> > static inline void hv_set_drvdata(struct hv_device *dev, void *data)
> > {
> > dev_set_drvdata(&dev->device, data);
> > --
> > 2.52.0
> >
>
^ permalink raw reply
* RE: [PATCH v2 2/4] Drivers: hv: Add logical device ID registry for vPCI devices
From: Michael Kelley @ 2026-07-08 13:28 UTC (permalink / raw)
To: Yu Zhang
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
iommu@lists.linux.dev, linux-pci@vger.kernel.org,
linux-arch@vger.kernel.org, wei.liu@kernel.org, kys@microsoft.com,
haiyangz@microsoft.com, decui@microsoft.com, longli@microsoft.com,
joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
bhelgaas@google.com, kwilczynski@kernel.org,
lpieralisi@kernel.org, mani@kernel.org, robh@kernel.org,
arnd@arndb.de, jgg@ziepe.ca, jacob.pan@linux.microsoft.com,
tgopinath@linux.microsoft.com,
easwar.hariharan@linux.microsoft.com, mrathor@linux.microsoft.com
In-Reply-To: <u2l4lz5skybcukrb6hwp2u6v3jdibrugokxmclgv3uq4ljj3vw@x7mlfvuwb5f4>
From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Wednesday, July 8, 2026 6:09 AM
>
> On Wed, Jul 08, 2026 at 02:52:43AM +0000, Michael Kelley wrote:
> > From: Yu Zhang <zhangyu1@linux.microsoft.com> Sent: Thursday, July 2, 2026 9:05 AM
> > >
> > > From: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> > >
> > > Hyper-V identifies each PCI pass-thru device by a logical device ID in
> > > its hypercall interface. This ID consists of a per-bus prefix, derived
> > > from the VMBus device instance GUID, combined with the PCI function
> > > number of the endpoint device.
> > >
> > > Add a small registry in hv_common.c that maps a PCI domain number to its
> > > logical device ID prefix. The vPCI bus driver (pci-hyperv) registers the
> > > prefix when a bus is probed and unregisters it when the bus is removed.
> > > Consumers such as the para-virtualized IOMMU driver look up the prefix
> > > by PCI domain number and combine it with the function number to form the
> > > complete logical device ID for hypercalls.
> > >
> > > The prefix construction is shared via hv_build_logical_dev_id_prefix() so
> > > that pci-hyperv's interrupt retargeting path and the registry use exactly
> > > the same byte layout. It is derived on demand from the constant hv_device
> > > instance GUID rather than cached in struct hv_pcibus_device, which is
> > > private to the pci-hyperv module; this keeps the interface narrow and
> > > avoids depending on pci-hyperv internals.
> > >
> > > Co-developed-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> > > Signed-off-by: Yu Zhang <zhangyu1@linux.microsoft.com>
> > > Signed-off-by: Easwar Hariharan <easwar.hariharan@linux.microsoft.com>
> > > ---
> > > drivers/hv/hv_common.c | 95 +++++++++++++++++++++++++++++
> > > drivers/pci/controller/pci-hyperv.c | 21 +++++--
> > > include/asm-generic/mshyperv.h | 13 ++++
> > > include/linux/hyperv.h | 8 +++
> > > 4 files changed, 132 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> > > index 6b67ac616789..53493f8d14dc 100644
> > > --- a/drivers/hv/hv_common.c
> > > +++ b/drivers/hv/hv_common.c
> > > @@ -26,6 +26,8 @@
> > > #include <linux/kmsg_dump.h>
> > > #include <linux/sizes.h>
> > > #include <linux/slab.h>
> > > +#include <linux/list.h>
> > > +#include <linux/spinlock.h>
> > > #include <linux/dma-map-ops.h>
> > > #include <linux/set_memory.h>
> > > #include <hyperv/hvhdk.h>
> > > @@ -863,3 +865,96 @@ const char *hv_result_to_string(u64 status)
> > > return "Unknown";
> > > }
> > > EXPORT_SYMBOL_GPL(hv_result_to_string);
> > > +
> > > +#ifdef CONFIG_HYPERV_PVIOMMU
> > > +/*
> > > + * Logical device ID registry shared between the vPCI bus driver
> > > + * (pci-hyperv) and the para-virtualized IOMMU driver. The vPCI driver
> > > + * registers the per-bus logical device ID prefix at bus probe time, and
> > > + * the pvIOMMU driver looks it up to build the full logical device ID used
> > > + * in IOMMU hypercalls.
> > > + */
> > > +struct hv_pci_busdata {
> > > + int pci_domain_nr;
> > > + u32 logical_dev_id_prefix;
> > > + struct list_head list;
> > > +};
> > > +
> > > +static LIST_HEAD(hv_pci_bus_list);
> > > +static DEFINE_SPINLOCK(hv_pci_bus_lock);
> > > +
> > > +int hv_iommu_register_pci_bus(int pci_domain_nr, u32 logical_dev_id_prefix)
> > > +{
> > > + struct hv_pci_busdata *bus, *new;
> > > + int ret = 0;
> > > +
> > > + new = kzalloc_obj(*new, GFP_KERNEL);
> > > + if (!new)
> > > + return -ENOMEM;
> > > +
> > > + spin_lock(&hv_pci_bus_lock);
> > > + list_for_each_entry(bus, &hv_pci_bus_list, list) {
> > > + if (bus->pci_domain_nr != pci_domain_nr)
> > > + continue;
> > > +
> > > + if (bus->logical_dev_id_prefix != logical_dev_id_prefix) {
> > > + pr_err("stale registration for PCI domain %d (old prefix 0x%08x, new 0x%08x)\n",
> > > + pci_domain_nr, bus->logical_dev_id_prefix,
> > > + logical_dev_id_prefix);
> > > + ret = -EEXIST;
> > > + }
> > > +
> > > + goto out_free;
> > > + }
> > > +
> > > + new->pci_domain_nr = pci_domain_nr;
> > > + new->logical_dev_id_prefix = logical_dev_id_prefix;
> > > + list_add(&new->list, &hv_pci_bus_list);
> > > + spin_unlock(&hv_pci_bus_lock);
> > > + return 0;
> > > +
> > > +out_free:
> > > + spin_unlock(&hv_pci_bus_lock);
> > > + kfree(new);
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_register_pci_bus, "pci-hyperv");
> > > +
> > > +void hv_iommu_unregister_pci_bus(int pci_domain_nr)
> > > +{
> > > + struct hv_pci_busdata *bus, *tmp;
> > > +
> > > + spin_lock(&hv_pci_bus_lock);
> > > + list_for_each_entry_safe(bus, tmp, &hv_pci_bus_list, list) {
> > > + if (bus->pci_domain_nr == pci_domain_nr) {
> > > + list_del(&bus->list);
> > > + kfree(bus);
> > > + break;
> > > + }
> > > + }
> > > + spin_unlock(&hv_pci_bus_lock);
> > > +}
> > > +EXPORT_SYMBOL_FOR_MODULES(hv_iommu_unregister_pci_bus, "pci-hyperv");
> > > +
> > > +/*
> > > + * Look up the logical device ID prefix registered for @pci_domain_nr.
> > > + * Returns 0 on success with *prefix filled in; -ENODEV if no entry is
> > > + * registered for that PCI domain.
> > > + */
> > > +int hv_iommu_lookup_logical_dev_id(int pci_domain_nr, u32 *prefix)
> > > +{
> > > + struct hv_pci_busdata *bus;
> > > + int ret = -ENODEV;
> > > +
> > > + spin_lock(&hv_pci_bus_lock);
> > > + list_for_each_entry(bus, &hv_pci_bus_list, list) {
> > > + if (bus->pci_domain_nr == pci_domain_nr) {
> > > + *prefix = bus->logical_dev_id_prefix;
> > > + ret = 0;
> > > + break;
> > > + }
> > > + }
> > > + spin_unlock(&hv_pci_bus_lock);
> > > + return ret;
> > > +}
> >
> > I started thinking about the mechanism here because it's somewhat
> > annoying that it takes 77 lines of code (sans comments) to manage
> > this simple little mapping. I also started thinking about how many entries
> > are likely to be in the mapping. A guest VM probably has fewer than 10
> > entries unless it has multiple NICs and maybe some GPUs. But this code
> > is also intended to be used by the Linux-as-root code, and I'm thinking
> > that the number of PCI devices managed by the root could easily be a
> > hundred or more if the root is managing a couple dozen VMs on a large
> > physical server. Searching a linked list with 100 or more entries could be
> > a bit slow.
> >
> > If only the guest scenario were needed, you could declare a static
> > array with 64 entries (64 is an arbitrary upper bound), and just search
> > through the array instead of having to allocate memory, deal with
> > allocation failures, and deal with linked lists. But a fixed size array
> > would need to be much bigger for the root scenario, and you would
> > still be doing a linear search.
> >
> > A better alternative to consider is rhashtable, which is an existing
> > Linux kernel facility. Based on what an AI bot generated for me, the
> > code for setting up and using rhashtable is straightforward, and
> > would probably result in far fewer than 77 lines of code. Lookups
> > would also be faster than a linear search, at least for the root case
> > with more than just a few entries. I'd suggest looking at rhashtable
> > to see whether you like how the resulting code comes out for this
> > use case, and whether it really is simpler than a roll-your-own linked
> > list.
> >
>
> Thank you so much for thinking this through, Michael! That is really
> a valid concern.
>
> How about using XArray? It might be more lightweight compared with
> rhashtable. Using pci_domain_nr as the key and prefix as the value.
> Maybe sth. like:
> ┃
> static DEFINE_XARRAY(hv_pci_bus_xa);
>
> int hv_iommu_register_pci_bus(int domain_nr, u32 prefix)
> {
> return xa_insert(&hv_pci_bus_xa, domain_nr,
> xa_mk_value(prefix), GFP_KERNEL);
> } ┃
>
> void hv_iommu_unregister_pci_bus(int domain_nr)
> {
> xa_erase(&hv_pci_bus_xa, domain_nr);
> }
>
> int hv_iommu_lookup_logical_dev_id(int domain_nr, u32 *prefix)
> {
> void *entry = xa_load(&hv_pci_bus_xa, domain_nr);
> if (!entry)
> ┃
> return -ENODEV;
> *prefix = xa_to_value(entry);
> return 0;
> ┃
> }
>
xarray is best where the keys are dense or mostly dense integers. In
this use case, the pci_domain_nr keys are essentially random 16-bit
values, which doesn't fit xarray as well. It would work, but wouldn't
be very efficient. See the 2nd paragraph of the documentation here:
https://www.kernel.org/doc/html/latest/core-api/xarray.html#xarray
Michael
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net v3 1/2] net: mana: Validate the packet length reported by the NIC
From: Dexuan Cui @ 2026-07-08 14:15 UTC (permalink / raw)
To: Paolo Abeni, KY Srinivasan, Haiyang Zhang, wei.liu@kernel.org,
Long Li, andrew+netdev@lunn.ch, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, Konstantin Taranov,
horms@kernel.org, ernis@linux.microsoft.com,
dipayanroy@linux.microsoft.com, kees@kernel.org,
jacob.e.keller@intel.com, ssengar@linux.microsoft.com,
linux-hyperv@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org
Cc: stable@vger.kernel.org
In-Reply-To: <d359508d-76a8-4df8-87ef-2767fe7fb40d@redhat.com>
> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Wednesday, July 8, 2026 3:57 AM
> ...
> On 7/2/26 6:12 AM, Dexuan Cui wrote:
> > Validate the packet length reported in the RX CQE before passing it
> > to skb processing. The CQE is supplied by the NIC device and should
> > not be blindly trusted.
> >
> > Cc: stable@vger.kernel.org
>
> This need a Fixes: tag, to help stable team backport.
Please use:
Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
In this first commit of the driver, mana_process_rx_cqe() only checks if pktlen
is not zero, and later mana_process_rx_cqe() -> mana_rx_skb() uses the
'pkt_len' blindly.
> No need to repost: just reply here, and I'll add it while applying the
> patch.
>
> Thanks,
>
> Paolo
Thank you, Paolo!
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH net-next v11 2/2] net: mana: force full-page RX buffers via ethtool private flag
From: Simon Horman @ 2026-07-08 15:57 UTC (permalink / raw)
To: dipayanroy
Cc: Simon Horman, kys, haiyangz, wei.liu, decui, andrew+netdev, davem,
edumazet, kuba, pabeni, leon, longli, kotaranov, shradhagupta,
ssengar, ernis, shirazsaleem, linux-hyperv, netdev, linux-kernel,
linux-rdma, stephen, jacob.e.keller, dipayanroy, leitao, kees,
john.fastabend, hawk, bpf, daniel, ast, sdf, yury.norov,
pavan.chebbi
In-Reply-To: <20260701141808.461554-3-dipayanroy@linux.microsoft.com>
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: mana: force full-page RX buffers via ethtool private flag
This adds an ethtool private flag "full-page-rx" that forces one RX buffer
per page to avoid a page_pool fragment path throughput regression on some
4K PAGE_SIZE ARM64 platforms. Toggling the flag while the port is up
triggers a mana_detach() + mana_attach() reconfiguration to apply the new
buffer layout.
A few questions inline.
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7438ea6b3f26..38f50b4eef87 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
[ ... mana_use_single_rxbuf_per_page() and mana_get_rxbuf_cfg() hunks
unrelated to the comments below ... ]
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 3531abfdaf43..24f6ce63b031 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
[ ... ]
> @@ -611,6 +630,88 @@ static int mana_get_link_ksettings(struct net_device *ndev,
> return 0;
> }
>
> +static u32 mana_get_priv_flags(struct net_device *ndev)
> +{
> + struct mana_port_context *apc = netdev_priv(ndev);
> +
> + return apc->priv_flags;
> +}
> +
> +static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)
> +{
> + struct mana_port_context *apc = netdev_priv(ndev);
> + u32 changed = apc->priv_flags ^ priv_flags;
> + u32 old_priv_flags = apc->priv_flags;
> + bool schedule_port_reset = false;
> + int err = 0;
> +
> + if (!changed)
> + return 0;
> +
> + /* Reject unknown bits */
> + if (priv_flags & ~GENMASK(MANA_PRIV_FLAG_MAX - 1, 0))
> + return -EINVAL;
> +
> + if (changed & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF)) {
> + apc->priv_flags = priv_flags;
[Low]
Is the storage of priv_flags intentionally nested inside the per-flag
branch? With only one flag today this is fine, but when a second bit is
added to enum mana_priv_flag_bits, a user toggling only the new bit
passes the !changed and unknown-bits checks, the outer if is skipped,
apc->priv_flags is never updated, and mana_set_priv_flags() returns 0
while mana_get_priv_flags() keeps reporting the old state.
Would it be cleaner to store priv_flags unconditionally after validation
and use per-flag if (changed & BIT(...)) blocks only for the
flag-specific reconfiguration?
> +
> + if (!apc->port_is_up) {
> + /* Port is down, flag updated to apply on next up
> + * so just return.
> + */
> + return 0;
> + }
> +
> + /* Pre-allocate buffers to prevent failure in mana_attach
> + * later
> + */
> + err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
[Low]
When the MTU is already jumbo (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2) or
XDP is attached, mana_use_single_rxbuf_per_page() already returns true
regardless of the new priv flag:
if (apc->priv_flags & BIT(MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF))
return true;
/* For xdp and jumbo frames make sure only one packet fits per page. */
if (mtu + MANA_RXBUF_PAD > PAGE_SIZE / 2 || mana_xdp_get(apc))
return true;
In that case mana_get_rxbuf_cfg() produces identical output whether the
new flag is on or off, yet toggling the flag here always performs a full
mana_pre_alloc_rxbufs() + mana_detach() + mana_attach() cycle and flaps
the link.
Would a pre/post comparison of mana_get_rxbuf_cfg() output be worth
adding to skip the reconfiguration when nothing actually changes?
[High]
mana_set_priv_flags() drives the same detach/attach sequence used by
mana_set_channels() and mana_per_port_queue_reset_work_handler(), but
does not take apc->vport_mutex and set apc->channel_changing = true
across the window.
The struct comment on channel_changing spells out the invariant:
/* Set by mana_set_channels() under vport_mutex to block RDMA
* from grabbing the vport during the detach/attach window.
* Checked by mana_cfg_vport() when called from the RDMA path.
*/
bool channel_changing;
Without it, after mana_detach() uncfg's the vport an RDMA client can
call mana_cfg_vport() and bump vport_use_count, and then mana_attach()
will fail with -EBUSY when it re-cfg's the vport.
There is also no equivalent of the '!port_is_up && vport_use_count'
early -EBUSY check that mana_set_channels() performs, so is the flag
allowed to be changed silently while an RDMA client is using the vport?
> + if (err) {
> + netdev_err(ndev,
> + "Insufficient memory for new allocations\n");
> + apc->priv_flags = old_priv_flags;
> + return err;
> + }
> +
> + err = mana_detach(ndev, false);
[High]
mana_detach() begins with ASSERT_RTNL(), and so does mana_attach()
called below. Is RTNL guaranteed to be held on the SET priv-flags path?
mana registers net_shaper_ops in mana_devops, which makes it an
ops-locked driver, so netdev_need_ops_lock() is true and the ethtool
core acquires rtnl only when the driver opts in via
ETHTOOL_OP_NEEDS_RTNL_* for that specific command.
Looking at mana_ethtool_ops.op_needs_rtnl:
.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
ETHTOOL_OP_NEEDS_RTNL_GLINK,
ETHTOOL_OP_NEEDS_RTNL_SPFLAGS is not listed, so ASSERT_RTNL() in the
detach/attach helpers below will WARN, and the code races with
RTNL-serialized mutators (mana_open, mana_close, mana_change_mtu,
mana_tx_timeout) that read and write apc->port_is_up, apc->rxqs,
apc->tx_qp and apc->port_st_save.
Should ETHTOOL_OP_NEEDS_RTNL_SPFLAGS be added to op_needs_rtnl, matching
what is already done for SET channels?
> + if (err) {
> + netdev_err(ndev, "mana_detach failed: %d\n", err);
> + apc->priv_flags = old_priv_flags;
> +
> + /* Port is in an inconsistent state. Restore
> + * 'port_is_up' so that queue reset work handler
> + * can properly detach and re-attach.
> + */
> + apc->port_is_up = true;
> + schedule_port_reset = true;
> + goto out;
> + }
> +
> + err = mana_attach(ndev);
> + if (err) {
> + netdev_err(ndev, "mana_attach failed: %d\n", err);
> + apc->priv_flags = old_priv_flags;
> +
> + /* Restore 'port_is_up' so the reset work handler
> + * can properly detach/attach. Without this,
> + * the handler sees port_is_up=false and skips
> + * queue allocation, leaving the port dead.
> + */
> + apc->port_is_up = true;
> + schedule_port_reset = true;
[High]
On this branch, mana_detach() has already succeeded and run
mana_cleanup_port_context(), so apc->rxqs is NULL and queues are torn
down. Setting apc->port_is_up = true here between now and when
queue_reset_work runs opens a window where callers gate on port_is_up
as their "safe to access rxqs/tx_qp" predicate.
For example, mana_get_ethtool_stats() does:
if (!apc->port_is_up)
return;
...
rxq = apc->rxqs[q];
...
A concurrent ethtool -S invocation during that window will pass the
port_is_up gate and dereference apc->rxqs[q]->stats on a NULL rxqs.
Is the port_is_up restore actually needed for the reset work handler's
mana_detach() call? On the reset path, mana_detach() takes its early
return when !netif_device_present(ndev):
if (!from_close && !netif_device_present(ndev))
return 0;
That early return does not touch apc->port_st_save, so the saved state
from the earlier successful mana_detach() should already be intact for
the follow-up mana_attach().
There is also no smp_wmb() paired with this write, unlike the pattern
used inside mana_detach()/mana_attach() proper.
> + }
> + }
> +
> +out:
> + mana_pre_dealloc_rxbufs(apc);
> +
> + if (schedule_port_reset)
> + queue_work(apc->ac->per_port_queue_reset_wq,
> + &apc->queue_reset_work);
> +
> + return err;
> +}
> +
[ ... remaining hunks unrelated to the comments above ... ]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox