[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber


  • To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 2 Mar 2022 11:34:44 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=0Lf0OBWBxnjnxgVBYGzhfgzUzdicXERsBIsCzj8mHTw=; b=Wh0irMgyVHkj7m0gD6TL9G/OR98alJx0H/im0u9IdcBKx8jSEqJwSrk3JlIKLc5+1YredQYADVoShEFmSOhJB7KhhYr6/FJRISVMsj8QUU/tL96iTk6dsCbxuwV5kxultVLR7oldJbXWPfhbIrShQAy4d4+4dYPM2BKdWjWRyi/K3KxvQio/RYi8+yKYCx74vPcSkyr2Hi9eOQCZ/jp3A/oQThU7SjW8/wlzXKr61Qw57kPEdPyvCayQdPHAe8wQOqpHUavORjl7aBf6iHXCjd/05ThmCAO9cXGeI7RRvQObUt9keK33DKTPmhtnln02QaZfsEFyEE9Rjc+8W4EcAg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=G0FZlPMbRNXySYYnbPD68a19cNjPiFKYwnq43HTwSENA11ZcQ5BU7i90ouFQuN5LoKmqj7tF1pkFAtqbjeBTRkVAbJs9n687yc3iL+JofhTBUW0vFNVzawJFtPHO/hu3bc7tPKY6MVvL2zmDnaBWJqAvE53kNkrGLRe96Fg0p617PYsgEEYzL5JIMSNk2RWxTw8uAYdu/rzsv7f51IFsdjCCXVA2WhI9sosenLAClXHtpyoHmY0jRKRSD4Yj+dkAKIIjlhBEpF29DLPoy6DPaCHgW+M7z3qNwxgdvbB4a0wTE/SvSDBfjZTnRkcnXo7jHk9+oPvMpTUfexLLxcb1sQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 02 Mar 2022 10:34:55 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.