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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, February 4, 2026 4:09 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; grygorii_strashko@xxxxxxxx; Andrew
> Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Andryuk, Jason <Jason.Andryuk@xxxxxxx>; Daniel P. Smith
> <dpsmith@xxxxxxxxxxxxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Garcia 
> Vallejo,
> Alejandro <Alejandro.GarciaVallejo@xxxxxxx>; Stabellini, Stefano
> <stefano.stabellini@xxxxxxx>
> Subject: Re: [PATCH v4 14/24] xen/domctl: wrap pci-subset iommu-related domctl
> op with CONFIG_MGMT_HYPERCALLS
>
> 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.
>

I may misunderstand the DCE process. With this change, we don't need to put 
#ifdef CONFIG_MGMT_HYPERCALLS around the statics (e.g. 
amd_iommu_assign_device()). So preprocessor will not help us remove them.
But if we enable --gc-sections flag, it will help us remove them in linker 
time, Without --gc-section, they will become unreachable codes.

> > 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.
>

Understood. Conditional NULL pointer fields are better be eliminated way down 
to the structure definition stage.

> Jan