Re: [PATCH v1 2/4] hyperv: Introduce new hypercall interfaces used by Hyper-V guest IOMMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Missing 4-byte padding in `struct hv_input_create_device_domain` resulting in an unaligned ABI structure size.
- [Low] Preprocessor macros are defined inside structure definitions.
--

commit 424b106dd2a0b2fe17c666987a01e49923b0b297
Author: Wei Liu <wei.liu@kernel.org>

hyperv: Introduce new hypercall interfaces used by Hyper-V guest IOMMU

This commit introduces new hypercall interfaces to support the Hyper-V guest
IOMMU, which allows child partitions to interact with the facility for device,
domain, and IOTLB management.

> diff --git a/include/hyperv/hvhdk_mini.h b/include/hyperv/hvhdk_mini.h
> index b4cb2fa26e9bc..493608e791b45 100644
> --- a/include/hyperv/hvhdk_mini.h
> +++ b/include/hyperv/hvhdk_mini.h

[ ... ]

> +struct hv_input_create_device_domain {
> +	struct hv_input_device_domain device_domain;
> +	union hv_create_device_domain_flags create_device_domain_flags;
> +} __packed;

Should this structure include a 4-byte padding field at the end?

Since the device_domain field is 24 bytes and the create_device_domain_flags
field is 4 bytes, the structure size is exactly 28 bytes. Other hypercall
structures in this file explicitly use padding to maintain an 8-byte aligned
size.

While this might work for a single invocation, if this structure is ever used
in a rep (array) hypercall, would the 28-byte stride misalign subsequent
elements and cause the hypercall to fail?

[ ... ]

> +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)

This isn't a bug, but should these preprocessor macros be moved outside of the
structure definition?

Macros do not respect C scope, so placing them inside the structure definition
violates standard kernel coding style conventions and might falsely imply they
are locally scoped.

> +
> +	u64 iommu_cap;
> +	u64 pgsize_bitmap;
> +} __packed;

[ ... ]

> +struct hv_output_get_logical_device_property {
> +#define HV_DEVICE_IOMMU_ENABLED (1ULL << 0)

Similarly, could this macro be moved outside the structure definition to adhere
to conventional kernel styling?

> +	u64 device_iommu;
> +	u64 reserved;
> +} __packed;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511162408.1180069-1-zhangyu1@linux.microsoft.com?part=2