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

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
  • Date: Tue, 22 Feb 2022 10:54:22 +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=GFaI6nSJKdNx7VzF9bzjMSUNMRy0c0wLRen1E+fsKDY=; b=Tlkialn8NpevDcIUzTW31Mv3q4k1QeMXntVsXh0m+YZo784kvb+8Mtw2xPVqzhaawOt3hUdLrFwISwFnbM64t6IeMDko63V0FnkSDVwdFhU3g1SQlW9XYR6t2HdWdBSax0x9dbsFWCfhXg3XrkV17+1SSylGOuVWIyXYls9DYVo+C2XPecJ6VhEddRq8mpg+eo+CZCw1Gdt7OUWIrWggoMAEGOSIRhZRhijEMW02vO206s5byWNqOmhT5zUl92mbaOTr7cHBsdCdAOPIunWD+hASEu/WqQC2EQJGdR26ww0YmH/nZ9LZA1orth/wVkxm0z/DjbeXylLccra7AheTPA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=J6dmgfTAx1zDfqssuht7HC9ufb1N6DYi3GyoVJDRlEDUiRLoMTvQns+zuMDgCPH4mlFqtz+HOEvNtUoXClNUepAQXEq9MoTDSdxbttsuHZ6gLqcjNubqQt0obBv6b95p8/M+jV8Tu0aTtilZ4epsaIT+H3j64N+OrhFbXRlnBowcF5C005vedKsxpoCMBRyLTMt0DNuGj53JVcdhZmbNe6f6PtrKz8OMz8CW4MPrGdaliKBGIRLYHyg2e8biwfNzDvYnji6YTob7TDO4vCIb/jYJpkzfzeIAZ6w2/Z0y1gTobWDZwKdn9o/H9ibZA6KRid/aJxtLmGeBE2FEyAv7ZQ==
  • Authentication-results: esa4.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: Tue, 22 Feb 2022 10:54:41 +0000
  • Ironport-data: A9a23:v3dIAqxb5j73/oRZMvZ6t+clxirEfRIJ4+MujC+fZmUNrF6WrkUOy TcYUDuCbKmIYjSmf9l/bI/k9UgOvpPcxtYwTQJoryAxQypGp/SeCIXCJC8cHc8zwu4v7q5Dx 59DAjUVBJlsFhcwnj/0bv656yMUOZigHtIQMsadUsxKbVIiGHdJZS5LwbZj2NYy24LhWWthh PupyyHhEA79s9JLGjp8B5Kr8HuDa9yr5Vv0FnRnDRx6lAe2e0s9VfrzFonoR5fMeaFGH/bSe gr25OrRElU1XfsaIojNfr7TKiXmS1NJVOSEoiI+t6OK2nCuqsGuu0qS2TV1hUp/0l20c95NJ Npltoe8c1xqIbPwvL49aRxbGAx/Jp1I0eqSSZS/mZT7I0zudnLtx7NlDV0sPJ1e8eFyaY1M3 aVGcnZXNEnF3r/ohuLgIgVvrp1LwM3DFYUToHx/ixreCu4rW8vrSKTW/95Imjw3g6iiGN6AO ZtEOGQ0NHwsZTVvHl0dUJAjvN6RuUClKx9/jlCLjoMOtj27IAtZj+G2bYu9lsaxbdVYmAOUq 3zL+0z9AwoGL5qPxDyd6HWui+TT2yThV+o6C7mQ5vNsxlqJyQQ7ChcbSF+6qvmRkVOlVpRUL El8x8Y1hfFsrgrxFIC7BkDm5i7f1vIBZzZOO+4h5DvWibXK3yLDJlIBTTVsaccWuuZjEFTGy WS1t9/uADVutpicRnSc6qqYoFuOBMQFEYMRTXRaFFVYurEPtKl210uSFYg7TMZZm/WoQWmY/ tyckMQpa1z/Z+Yv3r7zw13IiinESnPhHl9svVW/so5IA2pEiG+Zi26AtACzARVodt/xory9U J4swZP2AAcmV8zlqcB1aL9RdIxFHt7cWNEmvXZhHoM66xOm8GO5cIZb7VlWfRk1b5paKGK0O hOK4Wu9AaO/2lPwNsebhKrrVqwXIVXIT4y5Bpg4kPIUCnSOSON31H43PhPBt4wcuEMtjbs+K f+mnTWEVh4n5VBc5GPuHY81iOZzrghnnD+7bc2rnnyPjOvFDFbIGOhtDbd7Rr1ghE9yiF6Oq Ig32grj40g3bdASlQGNr9ZIdAhSdSJT6FKfg5U/S9Nv6zFOQQkJI/TQ3akga8pimaFUnf3P5 XazRglTz1+XuJENAVzihqxLAF83YatCkA==
  • Ironport-hdrordr: A9a23:lFLtTq3njdpy3Giuv8MF4AqjBRxyeYIsimQD101hICG9Lfb2qy n+ppgmPEHP5Qr5AEtQ5OxpOMG7MBbhHQYc2/hfAV7QZnibhILOFvAt0WKC+UytJ8SazIBgPM hbAtFD4bHLfDtHZIPBkXOF+rUbsZi6GcKT9J/jJh5WJGkAAcAB0+46MHfhLqQffngcOXNTLu v52iMznUvHRZ1hVLXdOpBqZZmgm/T70LbdJTIWDR8u7weDyRmy7qThLhSe1hACFxtS3LYL6w H+4kzEz5Tml8v+5g7X1mfV4ZgTssDm0MF/CMuFjdVQAinwizyveJ9qV9S5zXMISaCUmRQXee v30lMd1vdImjTsl6aO0F3QMjzboXMTArnZuAalaDXY0JTErXkBerV8bMpiA2XkAgwbzYtBOe twrhKkX9A8N2KwoA3to9fPTB1kjUyyvD4rlvMSlWVWVc8EZKZWtpF3xjIfLH4sJlOy1GkcKp gnMCgc3ocjTXqKK3TC+mV/yt2lWXo+Wh+AX0gZo8SQlzxbhmpwwUcUzNEW2i5ozuNxd7BUo+ Dfdqh4nrBHScEbKap7GecaWMOyTmjAWwjFPm6eKUnuUKsHJ3XOoZjq56hd3pDhRLUYiJ8p3J jRWlJRsmA/P0roFM2VxZVOtgvARW2sNA6dvP22J6IJzYEUaICbQxFrEmpe4PdIi89vd/HmZw ==
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYJ011tWngdwKc2UG3RuxDqKou56yfTukAgAAXlAA=
  • Thread-topic: [PATCH v2.1 8/7] x86/IOMMU: Use altcall, and __initconst_cf_clobber

On 22/02/2022 09:29, Jan Beulich wrote:
> On 21.02.2022 19:03, Andrew Cooper wrote:
>> Most IOMMU hooks are already altcall for performance reasons.  Convert the
>> rest of them so we can harden all the hooks in Control Flow Integrity
>> configurations.  This necessitates the use of iommu_{v,}call() in debug 
>> builds
>> too.
>>
>> Move the root iommu_ops from __read_mostly to __ro_after_init now that the
>> latter exists.  There is no need for a forward declaration of vtd_ops any
>> more, meaning that __initconst_cf_clobber can be used for VTD and AMD.
> The connection between the forward declaration and the annotation addition
> isn't really clear to me.
>
>> --- a/xen/arch/x86/include/asm/iommu.h
>> +++ b/xen/arch/x86/include/asm/iommu.h
>> @@ -72,7 +72,6 @@ struct arch_iommu
>>  
>>  extern struct iommu_ops iommu_ops;
>>  
>> -#ifdef NDEBUG
>>  # include <asm/alternative.h>
>>  # define iommu_call(ops, fn, args...) ({      \
>>      (void)(ops);                              \
>> @@ -83,7 +82,6 @@ extern struct iommu_ops iommu_ops;
>>      (void)(ops);                              \
>>      alternative_vcall(iommu_ops.fn, ## args); \
>>  })
>> -#endif
>>  
>>  static inline const struct iommu_ops *iommu_get_ops(void)
>>  {
>> @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *);
>>  static inline int iommu_adjust_irq_affinities(void)
>>  {
>>      return iommu_ops.adjust_irq_affinities
>> -           ? iommu_ops.adjust_irq_affinities()
>> +           ? iommu_call(iommu_ops, adjust_irq_affinities)
> While this (and other instances below) is x86-only code, where - with
> the removal of the #ifdef above - we now know the first argument is
> always ignored, I think it would still better be of the correct type
> (&iommu_ops). Perhaps the "(void)(ops)" in the macro definitions would
> better become "ASSERT((ops) == &iommu_ops)", which would check both
> type (compile time) and value (runtime).

I'm happy to fold that change if you want.  It ought to optimise out
completely for being

>
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -540,7 +540,7 @@ int __init iommu_setup(void)
>>  int iommu_suspend()
>>  {
>>      if ( iommu_enabled )
>> -        return iommu_get_ops()->suspend();
>> +        return iommu_call(iommu_get_ops(), suspend);
> This use of iommu_get_ops() in such constructs is a pattern we didn't
> have so far. Perhaps it just looks bogus, and all is fine in reality
> (apart from the whole idea being wrong for Arm, or really any
> environment where multiple dissimilar IOMMUs may be in use). Or wait,
> there are pre-existing cases (just not immediately visible when
> grep-ing for "iommu_v?call") in iommu_get_reserved_device_memory() and
> iommu_setup_hpet_msi().

I think this means your happy(ish) with the change?

I agree that this is nonsense on ARM, but the codepath isn't used yet
and someone's going to have to reconcile the conflicting views.

>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true;
>>  
>>  static unsigned int __read_mostly nr_iommus;
>>  
>> -static struct iommu_ops vtd_ops;
>>  static struct tasklet vtd_fault_tasklet;
>>  
>>  static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *);
>> @@ -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 = {
> Ah yes, the conversion to const (and the dropping of the forward decl)
> could have been part of "VT-d / x86: re-arrange cache syncing".
>
> With the missing &-s added and preferably with the description adjusted
> a little
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

~Andrew



 


Rackspace

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