Skip to content

Commit

Permalink
AMD/IOMMU: re-work locking around sending of commands
Browse files Browse the repository at this point in the history 
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. There's no real need for callers of that
function or of send_iommu_command() to hold the lock. Contain all
command sending related locking to the latter function.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul@xen.org>
  • Loading branch information
@jbeulich
jbeulich committed Jun 29, 2021
1 parent b175fc0 commit f95b7b3
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 38 deletions.
23 changes: 5 additions & 18 deletions xen/drivers/passthrough/amd/iommu_cmd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ static void send_iommu_command(struct amd_iommu *iommu,
const uint32_t cmd[4])
{
uint32_t tail;
unsigned long flags;

spin_lock_irqsave(&iommu->lock, flags);

tail = iommu->cmd_buffer.tail + sizeof(cmd_entry_t);
if ( tail == iommu->cmd_buffer.size )
Expand All @@ -47,6 +50,8 @@ static void send_iommu_command(struct amd_iommu *iommu,
iommu->cmd_buffer.tail = tail;

writel(tail, iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);

spin_unlock_irqrestore(&iommu->lock, flags);
}

static void flush_command_buffer(struct amd_iommu *iommu,
Expand Down Expand Up @@ -273,7 +278,6 @@ static void invalidate_iommu_all(struct amd_iommu *iommu)
void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev,
daddr_t daddr, unsigned int order)
{
unsigned long flags;
struct amd_iommu *iommu;
unsigned int req_id, queueid, maxpend;

Expand All @@ -300,10 +304,8 @@ void amd_iommu_flush_iotlb(u8 devfn, const struct pci_dev *pdev,
maxpend = pdev->ats.queue_depth & 0xff;

/* send INVALIDATE_IOTLB_PAGES command */
spin_lock_irqsave(&iommu->lock, flags);
invalidate_iotlb_pages(iommu, maxpend, 0, queueid, daddr, req_id, order);
flush_command_buffer(iommu, iommu_dev_iotlb_timeout);
spin_unlock_irqrestore(&iommu->lock, flags);
}

static void amd_iommu_flush_all_iotlbs(struct domain *d, daddr_t daddr,
Expand All @@ -330,17 +332,14 @@ static void amd_iommu_flush_all_iotlbs(struct domain *d, daddr_t daddr,
static void _amd_iommu_flush_pages(struct domain *d,
daddr_t daddr, unsigned int order)
{
unsigned long flags;
struct amd_iommu *iommu;
unsigned int dom_id = d->domain_id;

/* send INVALIDATE_IOMMU_PAGES command */
for_each_amd_iommu ( iommu )
{
spin_lock_irqsave(&iommu->lock, flags);
invalidate_iommu_pages(iommu, daddr, dom_id, order);
flush_command_buffer(iommu, 0);
spin_unlock_irqrestore(&iommu->lock, flags);
}

if ( ats_enabled )
Expand All @@ -360,37 +359,25 @@ void amd_iommu_flush_pages(struct domain *d,

void amd_iommu_flush_device(struct amd_iommu *iommu, uint16_t bdf)
{
ASSERT( spin_is_locked(&iommu->lock) );

invalidate_dev_table_entry(iommu, bdf);
flush_command_buffer(iommu, 0);
}

void amd_iommu_flush_intremap(struct amd_iommu *iommu, uint16_t bdf)
{
ASSERT( spin_is_locked(&iommu->lock) );

invalidate_interrupt_table(iommu, bdf);
flush_command_buffer(iommu, 0);
}

void amd_iommu_flush_all_caches(struct amd_iommu *iommu)
{
ASSERT( spin_is_locked(&iommu->lock) );

invalidate_iommu_all(iommu);
flush_command_buffer(iommu, 0);
}

void amd_iommu_send_guest_cmd(struct amd_iommu *iommu, u32 cmd[])
{
unsigned long flags;

spin_lock_irqsave(&iommu->lock, flags);

send_iommu_command(iommu, cmd);
/* TBD: Timeout selection may require peeking into cmd[]. */
flush_command_buffer(iommu, 0);

spin_unlock_irqrestore(&iommu->lock, flags);
}
3 changes: 2 additions & 1 deletion xen/drivers/passthrough/amd/iommu_guest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,9 +449,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
spin_lock_irqsave(&iommu->lock, flags);
dte_set_gcr3_table(mdte, hdom_id, gcr3_mfn << PAGE_SHIFT, gv, glx);

amd_iommu_flush_device(iommu, req_id);
spin_unlock_irqrestore(&iommu->lock, flags);

amd_iommu_flush_device(iommu, req_id);

return 0;
}

Expand Down
15 changes: 7 additions & 8 deletions xen/drivers/passthrough/amd/iommu_init.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,10 @@ static void enable_iommu(struct amd_iommu *iommu)
spin_lock_irqsave(&iommu->lock, flags);

if ( unlikely(iommu->enabled) )
goto out;
{
spin_unlock_irqrestore(&iommu->lock, flags);
return;
}

amd_iommu_erratum_746_workaround(iommu);

Expand Down Expand Up @@ -921,13 +924,12 @@ static void enable_iommu(struct amd_iommu *iommu)

set_iommu_translation_control(iommu, IOMMU_CONTROL_ENABLED);

if ( iommu->features.flds.ia_sup )
amd_iommu_flush_all_caches(iommu);

iommu->enabled = 1;

out:
spin_unlock_irqrestore(&iommu->lock, flags);

if ( iommu->features.flds.ia_sup )
amd_iommu_flush_all_caches(iommu);
}

static void disable_iommu(struct amd_iommu *iommu)
Expand Down Expand Up @@ -1544,7 +1546,6 @@ static int _invalidate_all_devices(
{
unsigned int bdf;
u16 req_id;
unsigned long flags;
struct amd_iommu *iommu;

for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ )
Expand All @@ -1553,10 +1554,8 @@ static int _invalidate_all_devices(
req_id = ivrs_mappings[bdf].dte_requestor_id;
if ( iommu )
{
spin_lock_irqsave(&iommu->lock, flags);
amd_iommu_flush_device(iommu, req_id);
amd_iommu_flush_intremap(iommu, req_id);
spin_unlock_irqrestore(&iommu->lock, flags);
}
}

Expand Down
6 changes: 0 additions & 6 deletions xen/drivers/passthrough/amd/iommu_intr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,7 @@ static int update_intremap_entry_from_ioapic(
entry.ptr32->flds.remap_en = false;
spin_unlock(lock);

spin_lock(&iommu->lock);
amd_iommu_flush_intremap(iommu, req_id);
spin_unlock(&iommu->lock);

spin_lock(lock);
}
Expand Down Expand Up @@ -527,11 +525,9 @@ static int update_intremap_entry_from_msi_msg(

if ( iommu->enabled )
{
spin_lock_irqsave(&iommu->lock, flags);
amd_iommu_flush_intremap(iommu, req_id);
if ( alias_id != req_id )
amd_iommu_flush_intremap(iommu, alias_id);
spin_unlock_irqrestore(&iommu->lock, flags);
}

return 0;
Expand Down Expand Up @@ -567,11 +563,9 @@ static int update_intremap_entry_from_msi_msg(
entry.ptr32->flds.remap_en = false;
spin_unlock(lock);

spin_lock(&iommu->lock);
amd_iommu_flush_intremap(iommu, req_id);
if ( alias_id != req_id )
amd_iommu_flush_intremap(iommu, alias_id);
spin_unlock(&iommu->lock);

spin_lock(lock);
}
Expand Down
15 changes: 10 additions & 5 deletions xen/drivers/passthrough/amd/pci_amd_iommu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ static void amd_iommu_setup_domain_device(
iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
dte->i = ats_enabled;

spin_unlock_irqrestore(&iommu->lock, flags);

amd_iommu_flush_device(iommu, req_id);

AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, type = %#x, "
Expand All @@ -138,8 +140,8 @@ static void amd_iommu_setup_domain_device(
page_to_maddr(hd->arch.amd.root_table),
domain->domain_id, hd->arch.amd.paging_mode);
}

spin_unlock_irqrestore(&iommu->lock, flags);
else
spin_unlock_irqrestore(&iommu->lock, flags);

ASSERT(pcidevs_locked());

Expand Down Expand Up @@ -307,14 +309,17 @@ static void amd_iommu_disable_domain_device(const struct domain *domain,
smp_wmb();
dte->v = true;

spin_unlock_irqrestore(&iommu->lock, flags);

amd_iommu_flush_device(iommu, req_id);

AMD_IOMMU_DEBUG("Disable: device id = %#x, "
"domain = %d, paging mode = %d\n",
req_id, domain->domain_id,
dom_iommu(domain)->arch.amd.paging_mode);
}
spin_unlock_irqrestore(&iommu->lock, flags);
else
spin_unlock_irqrestore(&iommu->lock, flags);

ASSERT(pcidevs_locked());

Expand Down Expand Up @@ -455,9 +460,9 @@ static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev)
iommu->dev_table.buffer + (bdf * IOMMU_DEV_TABLE_ENTRY_SIZE),
ivrs_mappings[bdf].intremap_table, iommu, iommu_intremap);

amd_iommu_flush_device(iommu, bdf);

spin_unlock_irqrestore(&iommu->lock, flags);

amd_iommu_flush_device(iommu, bdf);
}

amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev);
Expand Down

0 comments on commit f95b7b3

Please sign in to comment.