Re: [PATCH v4 14/24] xen/domctl: wrap pci-subset iommu-related domctl op with CONFIG_MGMT_HYPERCALLS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2026 08:50, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Penny, Zheng <penny.zheng@xxxxxxx>
>> Sent: Friday, November 21, 2025 6:58 PM
>>[...]
>> 772,14 +774,16 @@ static const struct iommu_ops __initconst_cf_clobber
>> _iommu_ops = {
>>      .quarantine_init = amd_iommu_quarantine_init,
>>      .add_device = amd_iommu_add_device,
>>      .remove_device = amd_iommu_remove_device,
>> -    .assign_device  = amd_iommu_assign_device,
>>      .teardown = amd_iommu_domain_destroy,
>>      .clear_root_pgtable = amd_iommu_clear_root_pgtable,
>>      .map_page = amd_iommu_map_page,
>>      .unmap_page = amd_iommu_unmap_page,
>>      .iotlb_flush = amd_iommu_flush_iotlb_pages,
>> +#ifdef CONFIG_MGMT_HYPERCALLS
>> +    .assign_device  = amd_iommu_assign_device,
>>      .reassign_device = reassign_device,
>>      .get_device_group_id = amd_iommu_group_id,
>> +#endif
> 
> FWIS, Alejandro has come up a more clever way to DCE these kinds of op, 
> staying conditionally as callback. Here, I just took this commit as example 
> to show the methodology:
> ```
>          .assign_device  = IS_ENABLED(CONFIG_MGMT_HYPERCALLS)
>                                         ? amd_iommu_assign_device
>                                         : NULL,
> ```
> The compiler has enough visibility to know that 
> static(amd_iommu_assign_device()) is used, and is droppable when 
> MGMT_HYPERCALLS=n. So there is no need to do ifdef-wrapping around these 
> statics now. Later when jason's "--gc-section" patch serie in,  --gc-section 
> will help linker identify them unused when MGMT_HYPERCALLS=n, then remove 
> them automatically.

I fear I don't see why --gc-sections would make a difference when, for static
functions, the compiler already is in the position of removing the functions.

> If we all agreed to use above methodology to do DCE.
> Alejandro also recommended that since we will do this assignments in enough 
> places in this patch serie, we probably want something like MAYBE_OP() 
> somewhere in xen/macros.h:
> 
> #define MAYBE_OP(c, fn) (IS_ENABLED(c) ? fn : NULL)
> 
> I'd like to listen from your opinions on whether I shall do such update for 
> v5, since it is quite a big update

Well, already there I did raise my concern of leaving around function pointer
fields in structures which will only ever be NULL. If respective fields are
removed altogether, there's no risk whatsoever that an accidental use may be
overlooked - the build would simply fail when making such an attempt. Calls
through NULL are privilege escalation XSAs when PV guests can somehow
leverage them, and use of altcall patching would still only downgrade them
to DoS XSAs.

Jan