On 02/03/2022 13:39, Andrew Cooper wrote:
> On 02/03/2022 10:34, Jan Beulich wrote:
>> On 02.03.2022 11:12, Andrew Cooper wrote:
>>> On 02/03/2022 08:10, Jan Beulich wrote:
>>>> On 01.03.2022 15:58, Andrew Cooper wrote:
>>>>> On 25/02/2022 08:24, Jan Beulich wrote:
>>>>>> On 22.02.2022 12:47, Andrew Cooper wrote:
>>>>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct
>>>>>>> domain *d)
>>>>>>> hd->arch.amd.paging_mode, 0, 0);
>>>>>>> }
>>>>>>>
>>>>>>> -static const struct iommu_ops __initconstrel _iommu_ops = {
>>>>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = {
>>>>>> Following my initcall related remark on x86'es time.c I'm afraid I don't
>>>>>> see how this and ...
>>>>>>
>>>>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check
>>>>>>> intel_iommu_quarantine_init(struct domain *d)
>>>>>>> return rc;
>>>>>>> }
>>>>>>>
>>>>>>> -static struct iommu_ops __initdata vtd_ops = {
>>>>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = {
>>>>>> ... this actually works. But I guess I must be overlooking something, as
>>>>>> I'm sure that you did test the change.
>>>>>>
>>>>>> Both ops structures reference a function, through .adjust_irq_affinities,
>>>>>> which isn't __init but which is used (besides here) for an initcall. With
>>>>>> the ENDBR removed by the time initcalls are run, these should cause #CP.
>>>>> This doesn't explode because the indirect calls are resolved to direct
>>>>> calls before the ENDBR's are clobbered to NOP4.
>>>> I'm afraid I don't understand: The problematic call is in do_initcalls():
>>>>
>>>> for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
>>>> (*call)();
>>>>
>>>> I don't see how this could be converted to a direct call.
>>> Oh. iov_adjust_irq_affinities()'s double use is hiding here.
>>>
>>> The safety rule for cf_clobber is that there must not be any
>>> non-alt-called callers. We need to fix it:
>>>
>>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
>>> b/xen/drivers/passthrough/amd/iommu_init.c
>>> index 657c7f619a51..b1af5085efda 100644
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void)
>>>
>>> return 0;
>>> }
>>> -__initcall(iov_adjust_irq_affinities);
>>> +
>>> +int cf_check __init initcall_iov_adjust_irq_affinities(void)
>>> +{
>>> + return iommu_call(&iommu_ops, adjust_irq_affinities);
>>> +}
>>> +__initcall(initcall_iov_adjust_irq_affinities);
>>>
>>> /*
>>> * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall
>>> Translations)
>>>
>>>
>>>> Afaics only pre-SMP initcalls are safe in this regard:
>>>> do_presmp_initcalls()
>>>> is called immediately ahead of alternative_branches().
>>>>
>>>> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable
>>>> retpolines with CET-IBT"?
>>> No. It's because AMD CPUs don't have CET-IBT at this juncture, and will
>>> never encounter a faulting situation.
>> I'm still lost. An exactly matching construct exists in VT-d code (and
>> my initial comment also was on VT-d). The AMD one is actually a clone
>> of that much older one. The initcall really wants to move to vendor
>> independent code, but I'd still like to understand why no fault was
>> ever observed.
> Lovely. It's got a vtd infix which is why it escaped my grep.
>
> And yes, I really would expect that to explode on my test system...
And the answer is that the linker script collects .init.rodata.* ahead
of the dedicated .init.rodata.cf_clobber section.
Meaning that __initdata_cf_clobber works as expected but
__initconst_cf_clobber is a no-op.
~Andrew
