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

Re: [PATCH 1/9] AMD/IOMMU: redo awaiting of command completion


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 10 Jun 2021 14:24:36 +0200
  • 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-SenderADCheck; bh=Dpw2Tc9J6gCifNz782V1DgMZt/Mp+Jf78OlFyyxmuT8=; b=dgPJmb6DgnsuseWidSMdfPidHf6b8mSy4ClDEET4MLdLAWxc9I+W/QbXtnOtPU/38PbqZUG4ddFizJkSNPgt2e4PSzZqanzqLR5yO/gSY9VYKocWPp5RWcNIo6b16GguxYhd/UocUlkE3ikywFfb7LT+UAWctJzSeJV4Zqidz6OIA7/LFPx7UWObwC6nQ1jucKYmNBaxOlnE1rgZoB5oP85LeR8RhF01ptanLS/iaGuUt3SEXfEZ/6enBONkEMopAublS3dqbLIWjM0iNALdQ1pgTMFWrODdXRHo+sxqEwSFZ0lEWRDt2YPRRPoKwupwcPf2nTMskemrRIArrw4Lww==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bpt+rlpaFxMGidkfPJHWmC+E0QYYx6is20OvEUvCRt29WRBtbFn6ZpBJuJtw8IrV5Ie1ZSNktjJSbF2b3rlXufEwr9nareKyFk+1LdROSKbyVPGNC9y9iKosDKtcR73kmnuvDBeVmzAv+z0KDSYLZ8Z0Tct+qAmfjy6K5C2mKWwUYx/TANt6Ox+12sZUQ56B6wMPPtortiW58GfWIQPpz+TgFdJx4qfb6i0c8vvOYqUhmvDBYkWcmYkvZm8bSuKtJy2TNlTtiO4LuL3BBC/3q7UEkKR+0KbQdGK9ivueT4R6KPJ6RS8FJjJev/ZtE+1UFCAz6Vu+8JUjlzIfy6ougg==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Paul Durrant <paul@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 10 Jun 2021 12:24:50 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 09.06.2021 12:36, Andrew Cooper wrote:
> On 09/06/2021 10:26, Jan Beulich wrote:
>> The present abuse of the completion interrupt does not only stand in the
>> way of, down the road, using it for its actual purpose, but also
>> requires holding the IOMMU lock while waiting for command completion,
>> limiting parallelism and keeping interrupts off for non-negligible
>> periods of time. Have the IOMMU do an ordinary memory write instead of
>> signaling an otherwise disabled interrupt (by just updating a status
>> register bit).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Reviewed-by: Paul Durrant <paul@xxxxxxx>
> 
> While I agree with the direction of the patch, some of the details could
> do with improvement.
> 
>>
>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>> @@ -20,6 +20,9 @@
>>  #include "iommu.h"
>>  #include "../ats.h"
>>  
>> +#define CMD_COMPLETION_INIT 0
>> +#define CMD_COMPLETION_DONE 1
>> +
>>  static void send_iommu_command(struct amd_iommu *iommu,
>>                                 const uint32_t cmd[4])
>>  {
>> @@ -49,28 +52,31 @@ static void send_iommu_command(struct am
>>  static void flush_command_buffer(struct amd_iommu *iommu,
>>                                   unsigned int timeout_base)
>>  {
>> +    static DEFINE_PER_CPU(uint64_t, poll_slot);
>> +    uint64_t *this_poll_slot = &this_cpu(poll_slot);
>> +    paddr_t addr = virt_to_maddr(this_poll_slot);
>>      uint32_t cmd[4];
>>      s_time_t start, timeout;
>>      static unsigned int __read_mostly threshold = 1;
>>  
>> -    /* RW1C 'ComWaitInt' in status register */
>> -    writel(IOMMU_STATUS_COMP_WAIT_INT,
>> -           iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>> -
>> -    /* send an empty COMPLETION_WAIT command to flush command buffer */
>> -    cmd[3] = cmd[2] = 0;
>> -    set_field_in_reg_u32(IOMMU_CMD_COMPLETION_WAIT, 0,
>> +    ACCESS_ONCE(*this_poll_slot) = CMD_COMPLETION_INIT;
>> +
>> +    /* send a COMPLETION_WAIT command to flush command buffer */
>> +    cmd[0] = addr;
>> +    set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, cmd[0],
>> +                         IOMMU_COMP_WAIT_S_FLAG_MASK,
>> +                         IOMMU_COMP_WAIT_S_FLAG_SHIFT, &cmd[0]);
> 
> set_field_in_reg_u32() is a disaster of a function - both in terms of
> semantics, and code gen - and needs to be purged from the code.
> 
> It is a shame we don't have a real struct for objects in the command
> buffer, but in lieu of that, this is just
> 
>     cmd[0] = addr | IOMMU_COMP_WAIT_S_FLAG_MASK;
> 
> which is the direction that previous cleanup has gone.
> 
> There are no current users of IOMMU_COMP_WAIT_S_FLAG_SHIFT, and ...
> 
>> +    cmd[1] = addr >> 32;
>> +    set_field_in_reg_u32(IOMMU_CMD_COMPLETION_WAIT, cmd[1],
>>                           IOMMU_CMD_OPCODE_MASK,
>>                           IOMMU_CMD_OPCODE_SHIFT, &cmd[1]);
>> -    set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
>> -                         IOMMU_COMP_WAIT_I_FLAG_MASK,
>> -                         IOMMU_COMP_WAIT_I_FLAG_SHIFT, &cmd[0]);
> 
> ... this drops the final use of IOMMU_COMP_WAIT_I_FLAG_SHIFT, so both
> should be dropped.
> 
> As for IOMMU_CMD_OPCODE_SHIFT, that can't be dropped yet, but it would
> still be better to use
> 
>     cmd[1] = (addr >> 32) | MASK_INSR(IOMMU_CMD_COMPLETION_WAIT,
> IOMMU_CMD_COMPLETION_WAIT);
> 
> in the short term.

Okay, this conversion has indeed saved a single

        and     $0x0FFFFFFF, %eax

But we're down by two set_field_in_reg_u32() now; only some 30 left.

Jan




 


Rackspace

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