From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul@xen.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/9] AMD/IOMMU: redo awaiting of command completion
Date: Thu, 10 Jun 2021 14:24:36 +0200 [thread overview]
Message-ID: <a6632eec-d64e-5167-2f0c-3ad919620327@suse.com> (raw)
In-Reply-To: <1fcb1140-b9b4-5c0b-de6c-e14d33937318@citrix.com>
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@suse.com>
>> Reviewed-by: Paul Durrant <paul@xen.org>
>
> 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
next prev parent reply other threads:[~2021-06-10 12:25 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 9:25 [PATCH 0/9] IOMMU: XSA-373 follow-on Jan Beulich
2021-06-09 9:26 ` [PATCH 1/9] AMD/IOMMU: redo awaiting of command completion Jan Beulich
2021-06-09 10:36 ` Andrew Cooper
2021-06-09 12:08 ` Jan Beulich
2021-06-09 12:33 ` Andrew Cooper
2021-06-09 12:51 ` Jan Beulich
2021-06-10 12:24 ` Jan Beulich [this message]
2021-06-09 9:27 ` [PATCH 2/9] AMD/IOMMU: re-work locking around sending of commands Jan Beulich
2021-06-09 10:53 ` Andrew Cooper
2021-06-09 12:22 ` Jan Beulich
2021-06-10 11:58 ` Jan Beulich
2021-06-10 12:53 ` Jan Beulich
2021-06-09 9:27 ` [PATCH 3/9] VT-d: undo device mappings upon error Jan Beulich
2021-06-24 5:13 ` Tian, Kevin
2021-06-09 9:28 ` [PATCH 4/9] VT-d: adjust domid map updating when unmapping context Jan Beulich
2021-06-24 5:21 ` Tian, Kevin
2021-06-09 9:28 ` [PATCH 5/9] VT-d: clear_fault_bits() should clear all fault bits Jan Beulich
2021-06-24 5:26 ` Tian, Kevin
2021-06-09 9:29 ` [PATCH 6/9] VT-d: don't lose errors when flushing TLBs on multiple IOMMUs Jan Beulich
2021-06-24 5:28 ` Tian, Kevin
2021-06-09 9:29 ` [PATCH 7/9] VT-d: centralize mapping of QI entries Jan Beulich
2021-06-24 5:31 ` Tian, Kevin
2021-06-09 9:29 ` [PATCH 8/9] VT-d: drop/move a few QI related constants Jan Beulich
2021-06-24 5:32 ` Tian, Kevin
2021-06-09 9:30 ` [PATCH 9/9] IOMMU/PCI: don't let domain cleanup continue when device de-assignment failed Jan Beulich
2021-06-24 15:34 ` Paul Durrant
2021-06-23 6:51 ` Ping: [PATCH 0/9] IOMMU: XSA-373 follow-on Jan Beulich
2021-06-23 6:58 ` Tian, Kevin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a6632eec-d64e-5167-2f0c-3ad919620327@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=paul@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).