Re: [PATCH 2/9] AMD/IOMMU: re-work locking around sending of commands

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.06.2021 12:53, Andrew Cooper wrote:
> On 09/06/2021 10:27, Jan Beulich wrote:
>> It appears unhelpful to me for flush_command_buffer() to block all
>> progress elsewhere for the given IOMMU by holding its lock while
>> waiting for command completion. Unless the lock is already held,
>> acquire it in send_iommu_command(). Release it in all cases in
>> flush_command_buffer(), before actually starting the wait loop.
>>
>> Some of the involved functions did/do get called with the lock already
>> held: For amd_iommu_flush_intremap() we can simply move the locking
>> inside. For amd_iommu_flush_device() and amd_iommu_flush_all_caches()
>> the lock now gets dropped in the course of the function's operation.
>>
>> Where touching function headers anyway, also adjust types used to be
>> better in line with our coding style and, where applicable, the
>> functions' callers.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Reviewed-by: Paul Durrant <paul@xxxxxxx>
> 
> Honestly, I'm -2 to this.  It is horrible obfuscation of the logic.
> 
> I agree with the premise of not holding the lock when we don't need to,
> but moving the lock/unlocks into different functions makes it impossible
> to follow.  (Also, the static analysers are going to scream at this
> patch, and rightfully so IMO.)
> 
> send_iommu_command() is static, as is flush_command_buffer(), so there
> is no need to split the locking like this AFAICT.
> 
> Instead, each amd_iommu_flush_* external accessor knows exactly what it
> is doing, and whether a wait descriptor is wanted. 
> flush_command_buffer() wants merging into send_iommu_command() as a
> "bool wait" parameter,

A further remark on this particular suggestion: While this is likely
doable, the result will presumably look a little odd: Besides the
various code paths calling send_iommu_command() and then
flush_command_buffer(), the former is also called _by_ the latter.
I can give this a try, but I'd like to be halfway certain I won't
be asked to undo that later on.

And of course this won't help with the split locking, only with some
of the passing around of the saved / to-be-restored eflags.

As an aside, the suggested "bool wait" parameter would (right now) only
ever get passed a "true" argument, so I'm not convinced it's useful to
have at this point, as then we'd also need to deal with the "false"
case (requiring a completion interrupt to be arranged for, which we
have no handler for) despite that code path being unused (and hence
also un-testable).

Jan