[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: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Wed, 2 Mar 2022 19:57:33 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=5HBJfX/YI4/vonTzojph5TRBFlUVz4e8zsRbbwbPR6Q=; b=oXRRmP6McKywJlMhD1HWcRlhxhAPHe3HygErfK8AgC7yK+gn4JQFhnDaz5IgY4Vb9Yv0Rlfsk9v3yfbDx6vIncRVoWnKqK2mhSuiQz098vSMZM5Sm/0/K3MAamdFfyYQVXZtVHpJ1rBwQduwCWTgzIhxNPzFwaVf2927ilW9QsT6x+W+brw8ptzH26GVhpXkxDAMW0N7pWf1sEG5yXRO/7iwscClXFc4U/WVK1ni6RhnY/Ppw1/qGrsjqC5QabkLALlv3V+LghZGSA4PkH6+vT654LrDyGSt/WrLw3KrposUlveKHzjlYdWMi4LWYHX+hDHPrrk+5KprrkHpFdoYcg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FqFFYSfXJ2RTFFVgjwgPt8jHwTEc+YJHxlV+YLRCITtiq8FngBgcrRAa74KvN8R7CSWTVDhC8AtPcjRy4HWA/SPMhy01a08uer/jeD2ck5MNRm0cFDBwJ4vOfRnqcdneHgur0BNAd+aUk7ZcUqztgF7s9xSX+BeiTdQs+wWdyWg+CT3x5LxMV1qg3yhLXpXTjVAbN20qmq/RHMqCHkPWDAezQmHCD974RA6Do/lJ6ng2BN6fM/lNTr0NbbqhV4ygCDeoGWOqYcv5UXugtPFHUJmw5HOH+WiODirwymHSugG4GWYHnUHQvgckXYQnTYv36kC93pGBi2qIMjDKvrWcyw==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 02 Mar 2022 19:58:08 +0000
  • Ironport-data: A9a23:LF8u6K4sr3evZ9a//ldKsAxRtCXHchMFZxGqfqrLsTDasY5as4F+v moeWz2BOquKYTb3eYpzao7g804Oup+AzNRjQVNrr31jHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuVGuG96yE6j8lkf5KkYAL+EnkZqTRMFWFw0XqPp8Zj2tQy2YPhWVvR0 T/Pi5a31GGNimYc3l08s8pvmDs31BglkGpF1rCWTakjUG72zxH5PrpGTU2CByKQrr1vNvy7X 47+IISRpQs1yfuP5uSNyd4XemVSKlLb0JPnZnB+A8BOiTAazsA+PzpS2FPxpi67hh3Q9+2dx umhurShTT8xP7/Uod0MCUdFCnh0PJxcoK3IdC3XXcy7lyUqclPpyvRqSko3IZcZ6qB8BmQmG f4wcW5XKErZ3qTvnez9GrIEascLdaEHOKs2vH16wC6fJvEhWZ3ZGI3B5MNC3Sd2jcdLdRrbT 5RFNmszMkSdC/FJEkYaN64/ntuDv3/YdjxkkBWQnYgY22eGmWSd15CyaYGIK7RmX/59jkue4 27L4Wn9KhUbL8CEjyqI9Gq2ge3Clj+9X5gdfJWn8tZ6jVvVwXYcYDUUX1ampfiyimalRslSb UcT/0ITQbMarRLxCIOnBlvh/SDC7kV0t8ds//MS+QW10qvG+z+gFzJaUycYYt0XrpckSml/v rOWpO/BCTtqubyTbHuS8LaIsD+/URQowX8+iTwsFlVcvYS6yG0npleWF4s4Tvbp5jHgMWyom 1i3QD4Ca6L/ZCLh/4Gy5hj5jj2lvfAlpSZlt1yMDgpJAu6UDbNJhrBEC3CGtZ6sz67DFzFtW UTofeDEtoji6rnXyUSwrB0lRu3B2hp8GGS0baRTN5cg7S+x3HWoYJpd5jpzTG8wbJpaIGGxO B6N4FILjHO2AJdMRfUtC25WI553pZUM6Py/DqyEBjawSsIZmPC7ENFGOhfLgjGFfLkEmqAjI 5aLGftA/l5BYZmLOAGeHr9HuZdyn3hW7TqKGfjTkkT2uZLDNSX9YepUbzOzghURsfrsTPP9q I0EaaNnCnx3DYXDX8Ug2dVLfABScCNiXsieRg4+XrfrHzeK0VoJUpf56bggZ5Zkj+JSkOLJ9 Wu6QUhW1Bz0gnivFOlAQikLhG/HNXqnkU8GAA==
  • Ironport-hdrordr: A9a23:15tRM6Dycq/OoBLlHegCsceALOsnbusQ8zAXPh9KJiC9I/b1qy nxppkmPEfP+UsssHFJo6HkBEEZKUmsuqKdkrNhQYtKOzOW9ldATbsSobcKpgePJ8SQzJ8l6U 4NSdkcNDS0NykBsS+Y2nj4Lz9D+qj+zEnAv463pB0NLT2CKZsQlDuRYjzrSXGeLzM2YabRYa DsgPav0ADQHkj/AP7LZEUtbqzmnZnmhZjmaRkJC1oM8w+Vlw6l77b8Dlyxwgoeeykn+8ZjzU H11yjCoomzufCyzRHRk0XJ6Y5NpdfnwtxfQOSRl8kuLCn2gArAXvUjZ1TChkF2nAic0idvrD D+mWZmAy210QKWQoiBm2qp5+An6kd215at8y7BvZKpm72HeNtzMbs+uWseSGqC16NohqAN7E oAtVjpxqZ/HFfOmj/w6MPPUAwvnk2ooWA6mepWlHBHV5ACAYUh5rD30XklWavoJhiKoLzP0d MeeP309bJTaxeXfnrZtm5gzJilWWkyBA6PRgwHttaO2zZbkXhlxw9ArfZv00so5dY4Ud1J9u 7EOqNnmPVHSdIXd7t0AKMETdGsAmLATBrQOCaZIEjhFqsAJ3XRwqSHqokd9aWvYtgF3ZEykJ POXBdRsnMzYVvnDYmU0JhC4nn2MROAtPTWu7ZjDrRCy8/BreDQQF6+oXgV4r6dn8k=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYJ+IV6PtRJY8p5U2wBnUH9jKzdqyj8n0AgAa3aoCAASBXAIAAIhyAgAAGJQCAADOGgIAAabgA
  • Thread-topic: [PATCH v2.2 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

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

 


Rackspace

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