2020-09-16 16:00:48

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: [PATCH 0/3] amd : iommu : Initial IOMMU support for SNP

Introducing support for AMD Secure Nested Paging (SNP) with IOMMU,
which mainly affects the use of IOMMU Exclusion Base and Range Limit
registers. Note that these registers are no longer used by Linux IOMMU
driver. Patch 2 and 3 are SNP-specific, and discuss detail of
the implementation.

In order to support SNP, the current Completion Wait Write-back logic
is modified (patch 1/4). This change is independent from SNP.

Please see the following white paper for more info on SNP:
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

Thank you,
Suravee

Suravee Suthikulpanit (3):
iommu: amd: Use 4K page for completion wait write-back semaphore
iommu: amd: Add support for RMP_PAGE_FAULT and RMP_HW_ERR
iommu: amd: Re-purpose Exclusion range registers to support SNP CWWB

drivers/iommu/amd/amd_iommu_types.h | 6 ++-
drivers/iommu/amd/init.c | 44 +++++++++++++++
drivers/iommu/amd/iommu.c | 84 ++++++++++++++++++++++++-----
3 files changed, 121 insertions(+), 13 deletions(-)

--
2.17.1


2020-09-16 18:01:48

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: [PATCH 1/3] iommu: amd: Use 4K page for completion wait write-back semaphore

IOMMU SNP support requires the completion wait write-back semaphore to be
implemented using a 4K-aligned page, where the page address is to be
programmed into the newly introduced MMIO base/range registers.

This new scheme uses a per-iommu atomic variable to store the current
semaphore value, which is incremented for every completion wait command.

Since this new scheme is also compatible with non-SNP mode,
generalize the driver to use 4K page for completion-wait semaphore in
both modes.

Cc: Brijesh Singh <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
drivers/iommu/amd/amd_iommu_types.h | 3 ++-
drivers/iommu/amd/init.c | 18 ++++++++++++++++++
drivers/iommu/amd/iommu.c | 23 +++++++++++------------
3 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 30a5d412255a..4c80483e78ec 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -595,7 +595,8 @@ struct amd_iommu {
#endif

u32 flags;
- volatile u64 __aligned(8) cmd_sem;
+ volatile u64 *cmd_sem;
+ u64 cmd_sem_val;

#ifdef CONFIG_AMD_IOMMU_DEBUGFS
/* DebugFS Info */
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 445a08d23fed..febc072f2717 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -813,6 +813,19 @@ static int iommu_init_ga(struct amd_iommu *iommu)
return ret;
}

+static int __init alloc_cwwb_sem(struct amd_iommu *iommu)
+{
+ iommu->cmd_sem = (void *)get_zeroed_page(GFP_KERNEL);
+
+ return iommu->cmd_sem ? 0 : -ENOMEM;
+}
+
+static void __init free_cwwb_sem(struct amd_iommu *iommu)
+{
+ if (iommu->cmd_sem)
+ free_page((unsigned long)iommu->cmd_sem);
+}
+
static void iommu_enable_xt(struct amd_iommu *iommu)
{
#ifdef CONFIG_IRQ_REMAP
@@ -1395,6 +1408,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu *iommu,

static void __init free_iommu_one(struct amd_iommu *iommu)
{
+ free_cwwb_sem(iommu);
free_command_buffer(iommu);
free_event_buffer(iommu);
free_ppr_log(iommu);
@@ -1481,6 +1495,7 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h)
int ret;

raw_spin_lock_init(&iommu->lock);
+ iommu->cmd_sem_val = 0;

/* Add IOMMU to internal data structures */
list_add_tail(&iommu->list, &amd_iommu_list);
@@ -1558,6 +1573,9 @@ static int __init init_iommu_one(struct amd_iommu *iommu, struct ivhd_header *h)
if (!iommu->mmio_base)
return -ENOMEM;

+ if (alloc_cwwb_sem(iommu))
+ return -ENOMEM;
+
if (alloc_command_buffer(iommu))
return -ENOMEM;

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 07ae8b93887e..a1d2c749a21f 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -792,11 +792,11 @@ irqreturn_t amd_iommu_int_handler(int irq, void *data)
*
****************************************************************************/

-static int wait_on_sem(volatile u64 *sem)
+static int wait_on_sem(struct amd_iommu *iommu, u64 data)
{
int i = 0;

- while (*sem == 0 && i < LOOP_TIMEOUT) {
+ while (*iommu->cmd_sem != data && i < LOOP_TIMEOUT) {
udelay(1);
i += 1;
}
@@ -827,16 +827,16 @@ static void copy_cmd_to_buffer(struct amd_iommu *iommu,
writel(tail, iommu->mmio_base + MMIO_CMD_TAIL_OFFSET);
}

-static void build_completion_wait(struct iommu_cmd *cmd, u64 address)
+static void build_completion_wait(struct iommu_cmd *cmd,
+ struct amd_iommu *iommu,
+ u64 data)
{
- u64 paddr = iommu_virt_to_phys((void *)address);
-
- WARN_ON(address & 0x7ULL);
+ u64 paddr = iommu_virt_to_phys((void *)iommu->cmd_sem);

memset(cmd, 0, sizeof(*cmd));
cmd->data[0] = lower_32_bits(paddr) | CMD_COMPL_WAIT_STORE_MASK;
cmd->data[1] = upper_32_bits(paddr);
- cmd->data[2] = 1;
+ cmd->data[2] = data;
CMD_SET_TYPE(cmd, CMD_COMPL_WAIT);
}

@@ -1045,22 +1045,21 @@ static int iommu_completion_wait(struct amd_iommu *iommu)
struct iommu_cmd cmd;
unsigned long flags;
int ret;
+ u64 data;

if (!iommu->need_sync)
return 0;

-
- build_completion_wait(&cmd, (u64)&iommu->cmd_sem);
-
raw_spin_lock_irqsave(&iommu->lock, flags);

- iommu->cmd_sem = 0;
+ data = ++iommu->cmd_sem_val;
+ build_completion_wait(&cmd, iommu, data);

ret = __iommu_queue_command_sync(iommu, &cmd, false);
if (ret)
goto out_unlock;

- ret = wait_on_sem(&iommu->cmd_sem);
+ ret = wait_on_sem(iommu, data);

out_unlock:
raw_spin_unlock_irqrestore(&iommu->lock, flags);
--
2.17.1

2020-09-16 19:05:47

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: [PATCH 2/3] iommu: amd: Add support for RMP_PAGE_FAULT and RMP_HW_ERR

IOMMU SNP support introduces two new IOMMU events:
* RMP Page Fault event
* RMP Hardware Error event

Hence, add reporting functions for these events.

Cc: Brijesh Singh <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
drivers/iommu/amd/amd_iommu_types.h | 2 +
drivers/iommu/amd/iommu.c | 61 +++++++++++++++++++++++++++++
2 files changed, 63 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 4c80483e78ec..1e7966c73707 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -128,6 +128,8 @@
#define EVENT_TYPE_IOTLB_INV_TO 0x7
#define EVENT_TYPE_INV_DEV_REQ 0x8
#define EVENT_TYPE_INV_PPR_REQ 0x9
+#define EVENT_TYPE_RMP_FAULT 0xd
+#define EVENT_TYPE_RMP_HW_ERR 0xe
#define EVENT_DEVID_MASK 0xffff
#define EVENT_DEVID_SHIFT 0
#define EVENT_DOMID_MASK_LO 0xffff
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a1d2c749a21f..73c035161f28 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -486,6 +486,61 @@ static void dump_command(unsigned long phys_addr)
pr_err("CMD[%d]: %08x\n", i, cmd->data[i]);
}

+static void amd_iommu_report_rmp_hw_error(volatile u32 *event)
+{
+ struct pci_dev *pdev;
+ struct iommu_dev_data *dev_data = NULL;
+ int devid = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
+ int vmg_tag = (event[1]) & 0xFFFF;
+ int flags = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
+ u64 spa = ((u64)event[3] << 32) | (event[2] & 0xFFFFFFF8);
+
+ pdev = pci_get_domain_bus_and_slot(0, PCI_BUS_NUM(devid),
+ devid & 0xff);
+ if (pdev)
+ dev_data = dev_iommu_priv_get(&pdev->dev);
+
+ if (dev_data && __ratelimit(&dev_data->rs)) {
+ pci_err(pdev, "Event logged [RMP_HW_ERROR devid=0x%04x, vmg_tag=0x%04x, spa=0x%llx, flags=0x%04x]\n",
+ devid, vmg_tag, spa, flags);
+ } else {
+ pr_err_ratelimited("Event logged [RMP_HW_ERROR device=%02x:%02x.%x, vmg_tag=0x%04x, spa=0x%llx, flags=0x%04x]\n",
+ PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+ vmg_tag, spa, flags);
+ }
+
+ if (pdev)
+ pci_dev_put(pdev);
+}
+
+static void amd_iommu_report_rmp_fault(volatile u32 *event)
+{
+ struct pci_dev *pdev;
+ struct iommu_dev_data *dev_data = NULL;
+ int devid = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
+ int flags_rmp = (event[0] >> EVENT_FLAGS_SHIFT) & 0xFF;
+ int vmg_tag = (event[1]) & 0xFFFF;
+ int flags = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
+ u64 gpa = ((u64)event[3] << 32) | event[2];
+
+ pdev = pci_get_domain_bus_and_slot(0, PCI_BUS_NUM(devid),
+ devid & 0xff);
+ if (pdev)
+ dev_data = dev_iommu_priv_get(&pdev->dev);
+
+ if (dev_data && __ratelimit(&dev_data->rs)) {
+ pci_err(pdev, "Event logged [RMP_PAGE_FAULT devid=0x%04x, vmg_tag=0x%04x, gpa=0x%llx, flags_rmp=0x%04x, flags=0x%04x]\n",
+ devid, vmg_tag, gpa, flags_rmp, flags);
+ } else {
+ pr_err_ratelimited("Event logged [RMP_PAGE_FAULT device=%02x:%02x.%x, vmg_tag=0x%04x, gpa=0x%llx, flags_rmp=0x%04x, flags=0x%04x]\n",
+ PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
+ vmg_tag, gpa, flags_rmp, flags);
+ }
+
+ if (pdev)
+ pci_dev_put(pdev);
+}
+
static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
u64 address, int flags)
{
@@ -577,6 +632,12 @@ static void iommu_print_event(struct amd_iommu *iommu, void *__evt)
PCI_BUS_NUM(devid), PCI_SLOT(devid), PCI_FUNC(devid),
pasid, address, flags);
break;
+ case EVENT_TYPE_RMP_FAULT:
+ amd_iommu_report_rmp_fault(event);
+ break;
+ case EVENT_TYPE_RMP_HW_ERR:
+ amd_iommu_report_rmp_hw_error(event);
+ break;
case EVENT_TYPE_INV_PPR_REQ:
pasid = PPR_PASID(*((u64 *)__evt));
tag = event[1] & 0x03FF;
--
2.17.1

2020-09-18 09:33:19

by Joerg Roedel

[permalink] [raw]
Subject: Re: [PATCH 2/3] iommu: amd: Add support for RMP_PAGE_FAULT and RMP_HW_ERR

Hi Suravee,

On Wed, Sep 16, 2020 at 01:55:48PM +0000, Suravee Suthikulpanit wrote:
> +static void amd_iommu_report_rmp_hw_error(volatile u32 *event)
> +{
> + struct pci_dev *pdev;
> + struct iommu_dev_data *dev_data = NULL;
> + int devid = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
> + int vmg_tag = (event[1]) & 0xFFFF;
> + int flags = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
> + u64 spa = ((u64)event[3] << 32) | (event[2] & 0xFFFFFFF8);

Please write this as:

struct iommu_dev_data *dev_data = NULL;
int devid, vmg_tag, flags;
struct pci_dev *pdev;
u64 spa;

devid = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
vmg_tag = (event[1]) & 0xFFFF;
flags = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
spa = ((u64)event[3] << 32) | (event[2] & 0xFFFFFFF8);

Same applied the the next function.

> +
> + pdev = pci_get_domain_bus_and_slot(0, PCI_BUS_NUM(devid),
> + devid & 0xff);
> + if (pdev)
> + dev_data = dev_iommu_priv_get(&pdev->dev);
> +
> + if (dev_data && __ratelimit(&dev_data->rs)) {
> + pci_err(pdev, "Event logged [RMP_HW_ERROR devid=0x%04x, vmg_tag=0x%04x, spa=0x%llx, flags=0x%04x]\n",
> + devid, vmg_tag, spa, flags);

Printing the devid is not really needed here, no? Same issue in the next
function.

Regards,

Joerg

2020-09-23 11:08:15

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: Re: [PATCH 2/3] iommu: amd: Add support for RMP_PAGE_FAULT and RMP_HW_ERR



On 9/18/20 4:31 PM, Joerg Roedel wrote:
> Hi Suravee,
>
> On Wed, Sep 16, 2020 at 01:55:48PM +0000, Suravee Suthikulpanit wrote:
>> +static void amd_iommu_report_rmp_hw_error(volatile u32 *event)
>> +{
>> + struct pci_dev *pdev;
>> + struct iommu_dev_data *dev_data = NULL;
>> + int devid = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
>> + int vmg_tag = (event[1]) & 0xFFFF;
>> + int flags = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
>> + u64 spa = ((u64)event[3] << 32) | (event[2] & 0xFFFFFFF8);
>
> Please write this as:
>
> struct iommu_dev_data *dev_data = NULL;
> int devid, vmg_tag, flags;
> struct pci_dev *pdev;
> u64 spa;
>
> devid = (event[0] >> EVENT_DEVID_SHIFT) & EVENT_DEVID_MASK;
> vmg_tag = (event[1]) & 0xFFFF;
> flags = (event[1] >> EVENT_FLAGS_SHIFT) & EVENT_FLAGS_MASK;
> spa = ((u64)event[3] << 32) | (event[2] & 0xFFFFFFF8);
>
> Same applied the the next function.
>
>> +
>> + pdev = pci_get_domain_bus_and_slot(0, PCI_BUS_NUM(devid),
>> + devid & 0xff);
>> + if (pdev)
>> + dev_data = dev_iommu_priv_get(&pdev->dev);
>> +
>> + if (dev_data && __ratelimit(&dev_data->rs)) {
>> + pci_err(pdev, "Event logged [RMP_HW_ERROR devid=0x%04x, vmg_tag=0x%04x, spa=0x%llx, flags=0x%04x]\n",
>> + devid, vmg_tag, spa, flags);
>
> Printing the devid is not really needed here, no? Same issue in the next
> function.

I'll update the patch and will send out V2.

Thanks,
Suravee