* [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local
2020-02-03 14:43 [Xen-devel] [PATCH 0/4] AMD/IOMMU: Cleanup Andrew Cooper
@ 2020-02-03 14:43 ` Andrew Cooper
2020-02-03 16:24 ` George Dunlap
2020-02-05 9:47 ` Jan Beulich
2020-02-03 14:43 ` [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers Andrew Cooper
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Andrew Cooper @ 2020-02-03 14:43 UTC (permalink / raw)
To: Xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monné
We currently have amd-iommu-defs.h, amd-iommu-proto.h and amd-iommu.h, and no
references outside of the AMD IOMMU driver.
Keep iommu-defs.h as is, but merge amd-iommu.h and amd-iommu-proto.h to just
iommu.h, and move them both into drivers/passthrough/amd/. (While merging,
drop the bogus #pragma pack around the *_entry structures.)
Take the opportunity to trim the include lists, including x86/mm/p2m.c
which (AFAICT) hasn't needed this include since c/s aef3f2275 "x86/mm/p2m:
break into common, pt-implementation and pod parts" in 2011.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
---
xen/arch/x86/mm/p2m.c | 1 -
.../passthrough/amd/iommu-defs.h} | 6 +-
.../passthrough/amd/iommu.h} | 179 ++++++++++++++++++-
xen/drivers/passthrough/amd/iommu_acpi.c | 7 +-
xen/drivers/passthrough/amd/iommu_cmd.c | 4 +-
xen/drivers/passthrough/amd/iommu_detect.c | 7 +-
xen/drivers/passthrough/amd/iommu_guest.c | 4 +-
xen/drivers/passthrough/amd/iommu_init.c | 13 +-
xen/drivers/passthrough/amd/iommu_intr.c | 9 +-
xen/drivers/passthrough/amd/iommu_map.c | 8 +-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 9 +-
xen/include/asm-x86/amd-iommu.h | 190 ---------------------
12 files changed, 194 insertions(+), 243 deletions(-)
rename xen/{include/asm-x86/hvm/svm/amd-iommu-defs.h => drivers/passthrough/amd/iommu-defs.h} (99%)
rename xen/{include/asm-x86/hvm/svm/amd-iommu-proto.h => drivers/passthrough/amd/iommu.h} (70%)
delete mode 100644 xen/include/asm-x86/amd-iommu.h
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index def13f657b..fd9f09536d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -38,7 +38,6 @@
#include <asm/mem_sharing.h>
#include <asm/hvm/nestedhvm.h>
#include <asm/altp2m.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
#include <asm/vm_event.h>
#include <xsm/xsm.h>
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h
similarity index 99%
rename from xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
rename to xen/drivers/passthrough/amd/iommu-defs.h
index 78368f16d9..f8b62cb033 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/drivers/passthrough/amd/iommu-defs.h
@@ -17,8 +17,8 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#ifndef _ASM_X86_64_AMD_IOMMU_DEFS_H
-#define _ASM_X86_64_AMD_IOMMU_DEFS_H
+#ifndef AMD_IOMMU_DEFS_H
+#define AMD_IOMMU_DEFS_H
/* IOMMU Command Buffer entries: in power of 2 increments, minimum of 256 */
#define IOMMU_CMD_BUFFER_DEFAULT_ENTRIES 512
@@ -506,7 +506,7 @@ struct amd_iommu_pte {
#define IOMMU_REG_BASE_ADDR_HIGH_MASK 0x000FFFFF
#define IOMMU_REG_BASE_ADDR_HIGH_SHIFT 0
-#endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */
+#endif /* AMD_IOMMU_DEFS_H */
/*
* Local variables:
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/drivers/passthrough/amd/iommu.h
similarity index 70%
rename from xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
rename to xen/drivers/passthrough/amd/iommu.h
index b5c0d50119..f590de8cbf 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -16,15 +16,180 @@
* You should have received a copy of the GNU General Public License
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-
-#ifndef _ASM_X86_64_AMD_IOMMU_PROTO_H
-#define _ASM_X86_64_AMD_IOMMU_PROTO_H
-
+#ifndef AMD_IOMMU_H
+#define AMD_IOMMU_H
+
+#include <xen/init.h>
+#include <xen/types.h>
+#include <xen/list.h>
+#include <xen/spinlock.h>
+#include <xen/tasklet.h>
#include <xen/sched.h>
-#include <asm/amd-iommu.h>
-#include <asm/apicdef.h>
#include <xen/domain_page.h>
+#include <asm/msi.h>
+#include <asm/apicdef.h>
+
+#include "iommu-defs.h"
+
+#define iommu_found() (!list_empty(&amd_iommu_head))
+
+extern struct list_head amd_iommu_head;
+
+typedef struct event_entry
+{
+ uint32_t data[4];
+} event_entry_t;
+
+typedef struct ppr_entry
+{
+ uint32_t data[4];
+} ppr_entry_t;
+
+typedef struct cmd_entry
+{
+ uint32_t data[4];
+} cmd_entry_t;
+
+struct table_struct {
+ void *buffer;
+ unsigned long entries;
+ unsigned long alloc_size;
+};
+
+struct ring_buffer {
+ void *buffer;
+ unsigned long entries;
+ unsigned long alloc_size;
+ uint32_t tail;
+ uint32_t head;
+ spinlock_t lock; /* protect buffer pointers */
+};
+
+typedef struct iommu_cap {
+ uint32_t header; /* offset 00h */
+ uint32_t base_low; /* offset 04h */
+ uint32_t base_hi; /* offset 08h */
+ uint32_t range; /* offset 0Ch */
+ uint32_t misc; /* offset 10h */
+} iommu_cap_t;
+
+struct amd_iommu {
+ struct list_head list;
+ spinlock_t lock; /* protect iommu */
+
+ u16 seg;
+ u16 bdf;
+ struct msi_desc msi;
+
+ u16 cap_offset;
+ iommu_cap_t cap;
+
+ u8 ht_flags;
+ union amd_iommu_ext_features features;
+
+ void *mmio_base;
+ unsigned long mmio_base_phys;
+
+ union amd_iommu_control ctrl;
+
+ struct table_struct dev_table;
+ struct ring_buffer cmd_buffer;
+ struct ring_buffer event_log;
+ struct ring_buffer ppr_log;
+
+ int exclusion_enable;
+ int exclusion_allow_all;
+ uint64_t exclusion_base;
+ uint64_t exclusion_limit;
+
+ int enabled;
+
+ struct list_head ats_devices;
+};
+
+struct ivrs_mappings {
+ uint16_t dte_requestor_id;
+ bool valid:1;
+ bool dte_allow_exclusion:1;
+ bool unity_map_enable:1;
+ bool write_permission:1;
+ bool read_permission:1;
+
+ /* ivhd device data settings */
+ uint8_t device_flags;
+
+ unsigned long addr_range_start;
+ unsigned long addr_range_length;
+ struct amd_iommu *iommu;
+
+ /* per device interrupt remapping table */
+ void *intremap_table;
+ unsigned long *intremap_inuse;
+ spinlock_t intremap_lock;
+};
+
+extern unsigned int ivrs_bdf_entries;
+extern u8 ivhd_type;
+
+struct ivrs_mappings *get_ivrs_mappings(u16 seg);
+int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
+int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
+ struct ivrs_mappings *, uint16_t));
+
+/* iommu tables in guest space */
+struct mmio_reg {
+ uint32_t lo;
+ uint32_t hi;
+};
+
+struct guest_dev_table {
+ struct mmio_reg reg_base;
+ uint32_t size;
+};
+
+struct guest_buffer {
+ struct mmio_reg reg_base;
+ struct mmio_reg reg_tail;
+ struct mmio_reg reg_head;
+ uint32_t entries;
+};
+
+struct guest_iommu_msi {
+ uint8_t vector;
+ uint8_t dest;
+ uint8_t dest_mode;
+ uint8_t delivery_mode;
+ uint8_t trig_mode;
+};
+
+/* virtual IOMMU structure */
+struct guest_iommu {
+
+ struct domain *domain;
+ spinlock_t lock;
+ bool_t enabled;
+
+ struct guest_dev_table dev_table;
+ struct guest_buffer cmd_buffer;
+ struct guest_buffer event_log;
+ struct guest_buffer ppr_log;
+
+ struct tasklet cmd_buffer_tasklet;
+
+ uint64_t mmio_base; /* MMIO base address */
+
+ /* MMIO regs */
+ union amd_iommu_control reg_ctrl; /* MMIO offset 0018h */
+ struct mmio_reg reg_status; /* MMIO offset 2020h */
+ union amd_iommu_ext_features reg_ext_feature; /* MMIO offset 0030h */
+
+ /* guest interrupt settings */
+ struct guest_iommu_msi msi;
+};
+
+extern bool_t iommuv2_enabled;
+
struct acpi_ivrs_hardware;
#define for_each_amd_iommu(amd_iommu) \
@@ -281,4 +446,4 @@ static inline void iommu_set_addr_hi_to_reg(uint32_t *reg, uint32_t addr)
IOMMU_REG_BASE_ADDR_HIGH_SHIFT, reg);
}
-#endif /* _ASM_X86_64_AMD_IOMMU_PROTO_H */
+#endif /* AMD_IOMMU_H */
diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index 6c5f8e46ec..f4abbfd9dc 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -17,13 +17,12 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/errno.h>
#include <xen/acpi.h>
#include <xen/param.h>
-#include <asm/apicdef.h>
+
#include <asm/io_apic.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
+
+#include "iommu.h"
/* Some helper structures, particularly to deal with ranges. */
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index af3a1fb865..92eaab407b 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -17,9 +17,7 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/sched.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
+#include "iommu.h"
#include "../ats.h"
static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
diff --git a/xen/drivers/passthrough/amd/iommu_detect.c b/xen/drivers/passthrough/amd/iommu_detect.c
index d782e66eee..8312bb4b6f 100644
--- a/xen/drivers/passthrough/amd/iommu_detect.c
+++ b/xen/drivers/passthrough/amd/iommu_detect.c
@@ -17,13 +17,10 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/errno.h>
#include <xen/acpi.h>
-#include <xen/iommu.h>
#include <xen/pci.h>
-#include <xen/pci_regs.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
+
+#include "iommu.h"
static int __init get_iommu_msi_capabilities(
u16 seg, u8 bus, u8 dev, u8 func, struct amd_iommu *iommu)
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 4ed6519e6e..aaf12fe1cb 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -16,11 +16,9 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/sched.h>
#include <asm/p2m.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
+#include "iommu.h"
#define IOMMU_MMIO_SIZE 0x8000
#define IOMMU_MMIO_PAGE_NR 0x8
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 2f26fed4a3..0ffc83a843 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -17,18 +17,11 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/errno.h>
#include <xen/acpi.h>
-#include <xen/keyhandler.h>
-#include <xen/pci.h>
-#include <xen/pci_regs.h>
-#include <xen/irq.h>
-#include <asm/amd-iommu.h>
-#include <asm/msi.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
-#include <asm-x86/fixmap.h>
-#include <mach_apic.h>
#include <xen/delay.h>
+#include <xen/keyhandler.h>
+
+#include "iommu.h"
static int __initdata nr_amd_iommus;
static bool __initdata pci_init;
diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c
index 5e92c023f8..e1cc13b873 100644
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -16,13 +16,12 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/err.h>
-#include <xen/sched.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
-#include <asm/io_apic.h>
#include <xen/softirq.h>
+#include <asm/io_apic.h>
+
+#include "iommu.h"
+
union irte32 {
uint32_t raw;
struct {
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 4e041b960f..2f3b47b366 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -18,12 +18,8 @@
*/
#include <xen/acpi.h>
-#include <xen/sched.h>
-#include <asm/p2m.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
-#include "../ats.h"
-#include <xen/pci.h>
+
+#include "iommu.h"
/* Given pfn and page table level, return pde index */
static unsigned int pfn_to_pde_idx(unsigned long pfn, unsigned int level)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index dd3401f0dc..3112653960 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -17,15 +17,12 @@
* along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
-#include <xen/sched.h>
#include <xen/iocap.h>
-#include <xen/pci.h>
-#include <xen/pci_regs.h>
-#include <xen/paging.h>
#include <xen/softirq.h>
+
#include <asm/acpi.h>
-#include <asm/amd-iommu.h>
-#include <asm/hvm/svm/amd-iommu-proto.h>
+
+#include "iommu.h"
#include "../ats.h"
static bool_t __read_mostly init_done;
diff --git a/xen/include/asm-x86/amd-iommu.h b/xen/include/asm-x86/amd-iommu.h
deleted file mode 100644
index 83ababdc8c..0000000000
--- a/xen/include/asm-x86/amd-iommu.h
+++ /dev/null
@@ -1,190 +0,0 @@
-/*
- * Copyright (C) 2007 Advanced Micro Devices, Inc.
- * Author: Leo Duran <leo.duran@amd.com>
- * Author: Wei Wang <wei.wang2@amd.com> - adapted to xen
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; If not, see <http://www.gnu.org/licenses/>.
- */
-#ifndef _ASM_X86_64_AMD_IOMMU_H
-#define _ASM_X86_64_AMD_IOMMU_H
-
-#include <xen/init.h>
-#include <xen/types.h>
-#include <xen/list.h>
-#include <xen/spinlock.h>
-#include <xen/tasklet.h>
-#include <asm/msi.h>
-#include <asm/hvm/svm/amd-iommu-defs.h>
-
-#define iommu_found() (!list_empty(&amd_iommu_head))
-
-extern struct list_head amd_iommu_head;
-
-#pragma pack(1)
-typedef struct event_entry
-{
- uint32_t data[4];
-} event_entry_t;
-
-typedef struct ppr_entry
-{
- uint32_t data[4];
-} ppr_entry_t;
-
-typedef struct cmd_entry
-{
- uint32_t data[4];
-} cmd_entry_t;
-#pragma pack()
-
-struct table_struct {
- void *buffer;
- unsigned long entries;
- unsigned long alloc_size;
-};
-
-struct ring_buffer {
- void *buffer;
- unsigned long entries;
- unsigned long alloc_size;
- uint32_t tail;
- uint32_t head;
- spinlock_t lock; /* protect buffer pointers */
-};
-
-typedef struct iommu_cap {
- uint32_t header; /* offset 00h */
- uint32_t base_low; /* offset 04h */
- uint32_t base_hi; /* offset 08h */
- uint32_t range; /* offset 0Ch */
- uint32_t misc; /* offset 10h */
-} iommu_cap_t;
-
-struct amd_iommu {
- struct list_head list;
- spinlock_t lock; /* protect iommu */
-
- u16 seg;
- u16 bdf;
- struct msi_desc msi;
-
- u16 cap_offset;
- iommu_cap_t cap;
-
- u8 ht_flags;
- union amd_iommu_ext_features features;
-
- void *mmio_base;
- unsigned long mmio_base_phys;
-
- union amd_iommu_control ctrl;
-
- struct table_struct dev_table;
- struct ring_buffer cmd_buffer;
- struct ring_buffer event_log;
- struct ring_buffer ppr_log;
-
- int exclusion_enable;
- int exclusion_allow_all;
- uint64_t exclusion_base;
- uint64_t exclusion_limit;
-
- int enabled;
-
- struct list_head ats_devices;
-};
-
-struct ivrs_mappings {
- uint16_t dte_requestor_id;
- bool valid:1;
- bool dte_allow_exclusion:1;
- bool unity_map_enable:1;
- bool write_permission:1;
- bool read_permission:1;
-
- /* ivhd device data settings */
- uint8_t device_flags;
-
- unsigned long addr_range_start;
- unsigned long addr_range_length;
- struct amd_iommu *iommu;
-
- /* per device interrupt remapping table */
- void *intremap_table;
- unsigned long *intremap_inuse;
- spinlock_t intremap_lock;
-};
-
-extern unsigned int ivrs_bdf_entries;
-extern u8 ivhd_type;
-
-struct ivrs_mappings *get_ivrs_mappings(u16 seg);
-int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
-int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
- struct ivrs_mappings *, uint16_t));
-
-/* iommu tables in guest space */
-struct mmio_reg {
- uint32_t lo;
- uint32_t hi;
-};
-
-struct guest_dev_table {
- struct mmio_reg reg_base;
- uint32_t size;
-};
-
-struct guest_buffer {
- struct mmio_reg reg_base;
- struct mmio_reg reg_tail;
- struct mmio_reg reg_head;
- uint32_t entries;
-};
-
-struct guest_iommu_msi {
- uint8_t vector;
- uint8_t dest;
- uint8_t dest_mode;
- uint8_t delivery_mode;
- uint8_t trig_mode;
-};
-
-/* virtual IOMMU structure */
-struct guest_iommu {
-
- struct domain *domain;
- spinlock_t lock;
- bool_t enabled;
-
- struct guest_dev_table dev_table;
- struct guest_buffer cmd_buffer;
- struct guest_buffer event_log;
- struct guest_buffer ppr_log;
-
- struct tasklet cmd_buffer_tasklet;
-
- uint64_t mmio_base; /* MMIO base address */
-
- /* MMIO regs */
- union amd_iommu_control reg_ctrl; /* MMIO offset 0018h */
- struct mmio_reg reg_status; /* MMIO offset 2020h */
- union amd_iommu_ext_features reg_ext_feature; /* MMIO offset 0030h */
-
- /* guest interrupt settings */
- struct guest_iommu_msi msi;
-};
-
-extern bool_t iommuv2_enabled;
-
-#endif /* _ASM_X86_64_AMD_IOMMU_H */
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local
2020-02-03 14:43 ` [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local Andrew Cooper
@ 2020-02-03 16:24 ` George Dunlap
2020-02-05 9:47 ` Jan Beulich
1 sibling, 0 replies; 21+ messages in thread
From: George Dunlap @ 2020-02-03 16:24 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: George Dunlap, Wei Liu, Roger Pau Monné
On 2/3/20 2:43 PM, Andrew Cooper wrote:
> We currently have amd-iommu-defs.h, amd-iommu-proto.h and amd-iommu.h, and no
> references outside of the AMD IOMMU driver.
>
> Keep iommu-defs.h as is, but merge amd-iommu.h and amd-iommu-proto.h to just
> iommu.h, and move them both into drivers/passthrough/amd/. (While merging,
> drop the bogus #pragma pack around the *_entry structures.)
>
> Take the opportunity to trim the include lists, including x86/mm/p2m.c
> which (AFAICT) hasn't needed this include since c/s aef3f2275 "x86/mm/p2m:
> break into common, pt-implementation and pod parts" in 2011.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> xen/arch/x86/mm/p2m.c | 1 -
> .../passthrough/amd/iommu-defs.h} | 6 +-
> .../passthrough/amd/iommu.h} | 179 ++++++++++++++++++-
> xen/drivers/passthrough/amd/iommu_acpi.c | 7 +-
> xen/drivers/passthrough/amd/iommu_cmd.c | 4 +-
> xen/drivers/passthrough/amd/iommu_detect.c | 7 +-
> xen/drivers/passthrough/amd/iommu_guest.c | 4 +-
> xen/drivers/passthrough/amd/iommu_init.c | 13 +-
> xen/drivers/passthrough/amd/iommu_intr.c | 9 +-
> xen/drivers/passthrough/amd/iommu_map.c | 8 +-
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 9 +-
> xen/include/asm-x86/amd-iommu.h | 190 ---------------------
> 12 files changed, 194 insertions(+), 243 deletions(-)
> rename xen/{include/asm-x86/hvm/svm/amd-iommu-defs.h => drivers/passthrough/amd/iommu-defs.h} (99%)
> rename xen/{include/asm-x86/hvm/svm/amd-iommu-proto.h => drivers/passthrough/amd/iommu.h} (70%)
> delete mode 100644 xen/include/asm-x86/amd-iommu.h
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index def13f657b..fd9f09536d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -38,7 +38,6 @@
> #include <asm/mem_sharing.h>
> #include <asm/hvm/nestedhvm.h>
> #include <asm/altp2m.h>
> -#include <asm/hvm/svm/amd-iommu-proto.h>
> #include <asm/vm_event.h>
> #include <xsm/xsm.h>
>
p2m bits:
Acked-by: George Dunlap <george.dunlap@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local
2020-02-03 14:43 ` [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local Andrew Cooper
2020-02-03 16:24 ` George Dunlap
@ 2020-02-05 9:47 ` Jan Beulich
2020-02-10 14:30 ` Andrew Cooper
1 sibling, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-02-05 9:47 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Xen-devel, Wei Liu, Roger Pau Monné
On 03.02.2020 15:43, Andrew Cooper wrote:
> We currently have amd-iommu-defs.h, amd-iommu-proto.h and amd-iommu.h, and no
> references outside of the AMD IOMMU driver.
>
> Keep iommu-defs.h as is, but merge amd-iommu.h and amd-iommu-proto.h to just
> iommu.h, and move them both into drivers/passthrough/amd/. (While merging,
> drop the bogus #pragma pack around the *_entry structures.)
>
> Take the opportunity to trim the include lists, including x86/mm/p2m.c
I guess you mean p2m.h here.
> which (AFAICT) hasn't needed this include since c/s aef3f2275 "x86/mm/p2m:
> break into common, pt-implementation and pod parts" in 2011.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local
2020-02-05 9:47 ` Jan Beulich
@ 2020-02-10 14:30 ` Andrew Cooper
2020-02-10 14:56 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-10 14:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Xen-devel, Wei Liu, Roger Pau Monné
On 05/02/2020 09:47, Jan Beulich wrote:
> On 03.02.2020 15:43, Andrew Cooper wrote:
>> We currently have amd-iommu-defs.h, amd-iommu-proto.h and amd-iommu.h, and no
>> references outside of the AMD IOMMU driver.
>>
>> Keep iommu-defs.h as is, but merge amd-iommu.h and amd-iommu-proto.h to just
>> iommu.h, and move them both into drivers/passthrough/amd/. (While merging,
>> drop the bogus #pragma pack around the *_entry structures.)
>>
>> Take the opportunity to trim the include lists, including x86/mm/p2m.c
> I guess you mean p2m.h here.
Why?
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index def13f657b..fd9f09536d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -38,7 +38,6 @@
> #include <asm/mem_sharing.h>
> #include <asm/hvm/nestedhvm.h>
> #include <asm/altp2m.h>
> -#include <asm/hvm/svm/amd-iommu-proto.h>
> #include <asm/vm_event.h>
> #include <xsm/xsm.h>
>
I really do mean p2m.c
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local
2020-02-10 14:30 ` Andrew Cooper
@ 2020-02-10 14:56 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2020-02-10 14:56 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Xen-devel, Wei Liu, Roger Pau Monné
On 10.02.2020 15:30, Andrew Cooper wrote:
> On 05/02/2020 09:47, Jan Beulich wrote:
>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>> We currently have amd-iommu-defs.h, amd-iommu-proto.h and amd-iommu.h, and no
>>> references outside of the AMD IOMMU driver.
>>>
>>> Keep iommu-defs.h as is, but merge amd-iommu.h and amd-iommu-proto.h to just
>>> iommu.h, and move them both into drivers/passthrough/amd/. (While merging,
>>> drop the bogus #pragma pack around the *_entry structures.)
>>>
>>> Take the opportunity to trim the include lists, including x86/mm/p2m.c
>> I guess you mean p2m.h here.
>
> Why?
>
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index def13f657b..fd9f09536d 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -38,7 +38,6 @@
>> #include <asm/mem_sharing.h>
>> #include <asm/hvm/nestedhvm.h>
>> #include <asm/altp2m.h>
>> -#include <asm/hvm/svm/amd-iommu-proto.h>
>> #include <asm/vm_event.h>
>> #include <xsm/xsm.h>
>>
>
> I really do mean p2m.c
Okay, I misunderstood then. I've been judging from
>--- a/xen/drivers/passthrough/amd/iommu_map.c
>+++ b/xen/drivers/passthrough/amd/iommu_map.c
>@@ -18,12 +18,8 @@
> */
>
> #include <xen/acpi.h>
>-#include <xen/sched.h>
>-#include <asm/p2m.h>
>-#include <asm/amd-iommu.h>
>-#include <asm/hvm/svm/amd-iommu-proto.h>
>-#include "../ats.h"
>-#include <xen/pci.h>
>+
>+#include "iommu.h"
where you _also_ (re)move p2m.h.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
2020-02-03 14:43 [Xen-devel] [PATCH 0/4] AMD/IOMMU: Cleanup Andrew Cooper
2020-02-03 14:43 ` [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local Andrew Cooper
@ 2020-02-03 14:43 ` Andrew Cooper
2020-02-05 9:57 ` Jan Beulich
2020-02-03 14:43 ` [Xen-devel] [PATCH 3/4] AMD/IOMMU: Treat guest head/tail pointers as byte offsets Andrew Cooper
2020-02-03 14:43 ` [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat " Andrew Cooper
3 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-03 14:43 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
These are obfuscations around simple bit operations, and the compiler really
can do a better job when it can see them normally:
add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-181 (-181)
Function old new delta
guest_iommu_add_ppr_log 266 251 -15
guest_iommu_add_event_log 266 251 -15
iommu_reset_log 274 250 -24
guest_iommu_process_command 1602 1544 -58
guest_iommu_mmio_write 1123 1054 -69
Total: Before=3014099, After=3013918, chg -0.01%
Drop all status register MASK/SHIFT constants, and enumerate the bits
normally. Rename EVENT_OVERFLOW to EVENT_LOG_OVERFLOW for consistency. (The
field name in the spec is inconsistent, despite the description referring to
an overflow of the event log.)
The only semantic change is in iommu_reset_log() where 'run_bit' changes from
being a bit position to being a single-bit mask.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
xen/drivers/passthrough/amd/iommu-defs.h | 34 +++++++++---------------
xen/drivers/passthrough/amd/iommu.h | 15 -----------
xen/drivers/passthrough/amd/iommu_cmd.c | 8 +++---
xen/drivers/passthrough/amd/iommu_guest.c | 43 +++++++++++++------------------
xen/drivers/passthrough/amd/iommu_init.c | 21 +++++++--------
5 files changed, 43 insertions(+), 78 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h
index f8b62cb033..963009de6a 100644
--- a/xen/drivers/passthrough/amd/iommu-defs.h
+++ b/xen/drivers/passthrough/amd/iommu-defs.h
@@ -437,28 +437,18 @@ union amd_iommu_x2apic_control {
/* Status Register*/
#define IOMMU_STATUS_MMIO_OFFSET 0x2020
-#define IOMMU_STATUS_EVENT_OVERFLOW_MASK 0x00000001
-#define IOMMU_STATUS_EVENT_OVERFLOW_SHIFT 0
-#define IOMMU_STATUS_EVENT_LOG_INT_MASK 0x00000002
-#define IOMMU_STATUS_EVENT_LOG_INT_SHIFT 1
-#define IOMMU_STATUS_COMP_WAIT_INT_MASK 0x00000004
-#define IOMMU_STATUS_COMP_WAIT_INT_SHIFT 2
-#define IOMMU_STATUS_EVENT_LOG_RUN_MASK 0x00000008
-#define IOMMU_STATUS_EVENT_LOG_RUN_SHIFT 3
-#define IOMMU_STATUS_CMD_BUFFER_RUN_MASK 0x00000010
-#define IOMMU_STATUS_CMD_BUFFER_RUN_SHIFT 4
-#define IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK 0x00000020
-#define IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT 5
-#define IOMMU_STATUS_PPR_LOG_INT_MASK 0x00000040
-#define IOMMU_STATUS_PPR_LOG_INT_SHIFT 6
-#define IOMMU_STATUS_PPR_LOG_RUN_MASK 0x00000080
-#define IOMMU_STATUS_PPR_LOG_RUN_SHIFT 7
-#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW_MASK 0x00000100
-#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW_SHIFT 8
-#define IOMMU_STATUS_GAPIC_LOG_INT_MASK 0x00000200
-#define IOMMU_STATUS_GAPIC_LOG_INT_SHIFT 9
-#define IOMMU_STATUS_GAPIC_LOG_RUN_MASK 0x00000400
-#define IOMMU_STATUS_GAPIC_LOG_RUN_SHIFT 10
+
+#define IOMMU_STATUS_EVENT_LOG_OVERFLOW 0x00000001
+#define IOMMU_STATUS_EVENT_LOG_INT 0x00000002
+#define IOMMU_STATUS_COMP_WAIT_INT 0x00000004
+#define IOMMU_STATUS_EVENT_LOG_RUN 0x00000008
+#define IOMMU_STATUS_CMD_BUFFER_RUN 0x00000010
+#define IOMMU_STATUS_PPR_LOG_OVERFLOW 0x00000020
+#define IOMMU_STATUS_PPR_LOG_INT 0x00000040
+#define IOMMU_STATUS_PPR_LOG_RUN 0x00000080
+#define IOMMU_STATUS_GAPIC_LOG_OVERFLOW 0x00000100
+#define IOMMU_STATUS_GAPIC_LOG_INT 0x00000200
+#define IOMMU_STATUS_GAPIC_LOG_RUN 0x00000400
/* I/O Page Table */
#define IOMMU_PAGE_TABLE_ENTRY_SIZE 8
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index f590de8cbf..81b6812d3a 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -374,21 +374,6 @@ static inline void __free_amd_iommu_tables(void *table, int order)
free_xenheap_pages(table, order);
}
-static inline void iommu_set_bit(uint32_t *reg, uint32_t bit)
-{
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, *reg, 1U << bit, bit, reg);
-}
-
-static inline void iommu_clear_bit(uint32_t *reg, uint32_t bit)
-{
- set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *reg, 1U << bit, bit, reg);
-}
-
-static inline uint32_t iommu_get_bit(uint32_t reg, uint32_t bit)
-{
- return get_field_from_reg_u32(reg, 1U << bit, bit);
-}
-
static inline int iommu_has_cap(struct amd_iommu *iommu, uint32_t bit)
{
return !!(iommu->cap.header & (1u << bit));
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 92eaab407b..166f0e7263 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -68,7 +68,7 @@ static void flush_command_buffer(struct amd_iommu *iommu)
int loop_count, comp_wait;
/* RW1C 'ComWaitInt' in status register */
- writel(IOMMU_STATUS_COMP_WAIT_INT_MASK,
+ writel(IOMMU_STATUS_COMP_WAIT_INT,
iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
/* send an empty COMPLETION_WAIT command to flush command buffer */
@@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu *iommu)
loop_count = 1000;
do {
status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
- comp_wait = get_field_from_reg_u32(status,
- IOMMU_STATUS_COMP_WAIT_INT_MASK,
- IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
+ comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
--loop_count;
} while ( !comp_wait && loop_count );
if ( comp_wait )
{
/* RW1C 'ComWaitInt' in status register */
- writel(IOMMU_STATUS_COMP_WAIT_INT_MASK,
+ writel(IOMMU_STATUS_COMP_WAIT_INT,
iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
return;
}
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index aaf12fe1cb..d05901d348 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -30,12 +30,6 @@
#define GUEST_ADDRESS_SIZE_6_LEVEL 0x2
#define HOST_ADDRESS_SIZE_6_LEVEL 0x2
-#define guest_iommu_set_status(iommu, bit) \
- iommu_set_bit(&((iommu)->reg_status.lo), bit)
-
-#define guest_iommu_clear_status(iommu, bit) \
- iommu_clear_bit(&((iommu)->reg_status.lo), bit)
-
#define reg_to_u64(reg) (((uint64_t)reg.hi << 32) | reg.lo )
#define u64_to_reg(reg, val) \
do \
@@ -183,7 +177,7 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
if ( ++tail >= iommu->ppr_log.entries )
{
tail = 0;
- guest_iommu_set_status(iommu, IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT);
+ iommu->reg_status.lo |= IOMMU_STATUS_PPR_LOG_OVERFLOW;
}
iommu_set_rb_pointer(&iommu->ppr_log.reg_tail.lo, tail);
unmap_domain_page(log_base);
@@ -231,7 +225,7 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
if ( ++tail >= iommu->event_log.entries )
{
tail = 0;
- guest_iommu_set_status(iommu, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT);
+ iommu->reg_status.lo |= IOMMU_STATUS_EVENT_LOG_OVERFLOW;
}
iommu_set_rb_pointer(&iommu->event_log.reg_tail.lo, tail);
@@ -322,11 +316,11 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
iommu = domain_iommu(d);
- i = iommu_get_bit(cmd->data[0], IOMMU_COMP_WAIT_I_FLAG_SHIFT);
- s = iommu_get_bit(cmd->data[0], IOMMU_COMP_WAIT_S_FLAG_SHIFT);
+ i = cmd->data[0] & IOMMU_COMP_WAIT_I_FLAG_MASK;
+ s = cmd->data[0] & IOMMU_COMP_WAIT_S_FLAG_MASK;
if ( i )
- guest_iommu_set_status(iommu, IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
+ iommu->reg_status.lo |= IOMMU_STATUS_COMP_WAIT_INT;
if ( s )
{
@@ -352,8 +346,7 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
unmap_domain_page(vaddr);
}
- com_wait_int = iommu_get_bit(iommu->reg_status.lo,
- IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
+ com_wait_int = iommu->reg_status.lo & IOMMU_STATUS_COMP_WAIT_INT;
if ( iommu->reg_ctrl.com_wait_int_en && com_wait_int )
guest_iommu_deliver_msi(d);
@@ -539,16 +532,16 @@ static int guest_iommu_write_ctrl(struct guest_iommu *iommu, uint64_t val)
{
guest_iommu_enable_ring_buffer(iommu, &iommu->event_log,
sizeof(event_entry_t));
- guest_iommu_set_status(iommu, IOMMU_STATUS_EVENT_LOG_RUN_SHIFT);
- guest_iommu_clear_status(iommu, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT);
+ iommu->reg_status.lo |= IOMMU_STATUS_EVENT_LOG_RUN;
+ iommu->reg_status.lo &= ~IOMMU_STATUS_EVENT_LOG_OVERFLOW;
}
if ( newctrl.iommu_en && newctrl.ppr_en && newctrl.ppr_log_en )
{
guest_iommu_enable_ring_buffer(iommu, &iommu->ppr_log,
sizeof(ppr_entry_t));
- guest_iommu_set_status(iommu, IOMMU_STATUS_PPR_LOG_RUN_SHIFT);
- guest_iommu_clear_status(iommu, IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT);
+ iommu->reg_status.lo |= IOMMU_STATUS_PPR_LOG_RUN;
+ iommu->reg_status.lo &= ~IOMMU_STATUS_PPR_LOG_OVERFLOW;
}
if ( newctrl.iommu_en && iommu->reg_ctrl.cmd_buf_en &&
@@ -559,7 +552,7 @@ static int guest_iommu_write_ctrl(struct guest_iommu *iommu, uint64_t val)
}
if ( iommu->reg_ctrl.event_log_en && !newctrl.event_log_en )
- guest_iommu_clear_status(iommu, IOMMU_STATUS_EVENT_LOG_RUN_SHIFT);
+ iommu->reg_status.lo &= ~IOMMU_STATUS_EVENT_LOG_RUN;
if ( iommu->reg_ctrl.iommu_en && !newctrl.iommu_en )
guest_iommu_disable(iommu);
@@ -698,13 +691,13 @@ static void guest_iommu_mmio_write64(struct guest_iommu *iommu,
u64_to_reg(&iommu->ppr_log.reg_tail, val);
break;
case IOMMU_STATUS_MMIO_OFFSET:
- val &= IOMMU_STATUS_EVENT_OVERFLOW_MASK |
- IOMMU_STATUS_EVENT_LOG_INT_MASK |
- IOMMU_STATUS_COMP_WAIT_INT_MASK |
- IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK |
- IOMMU_STATUS_PPR_LOG_INT_MASK |
- IOMMU_STATUS_GAPIC_LOG_OVERFLOW_MASK |
- IOMMU_STATUS_GAPIC_LOG_INT_MASK;
+ val &= IOMMU_STATUS_EVENT_LOG_OVERFLOW |
+ IOMMU_STATUS_EVENT_LOG_INT |
+ IOMMU_STATUS_COMP_WAIT_INT |
+ IOMMU_STATUS_PPR_LOG_OVERFLOW |
+ IOMMU_STATUS_PPR_LOG_INT |
+ IOMMU_STATUS_GAPIC_LOG_OVERFLOW |
+ IOMMU_STATUS_GAPIC_LOG_INT;
u64_to_reg(&iommu->reg_status, reg_to_u64(iommu->reg_status) & ~val);
break;
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 0ffc83a843..7bf6fef3ee 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
run_bit = ( log == &iommu->event_log ) ?
- IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
- IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
+ IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
/* wait until EventLogRun bit = 0 */
do {
entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
- log_run = iommu_get_bit(entry, run_bit);
+ log_run = entry & run_bit;
loop_count--;
} while ( log_run && loop_count );
@@ -371,8 +370,8 @@ static void iommu_reset_log(struct amd_iommu *iommu,
ctrl_func(iommu, IOMMU_CONTROL_DISABLED);
/* RW1C overflow bit */
- writel(log == &iommu->event_log ? IOMMU_STATUS_EVENT_OVERFLOW_MASK
- : IOMMU_STATUS_PPR_LOG_OVERFLOW_MASK,
+ writel(log == &iommu->event_log ? IOMMU_STATUS_EVENT_LOG_OVERFLOW
+ : IOMMU_STATUS_PPR_LOG_OVERFLOW,
iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
/*reset event log base address */
@@ -589,7 +588,7 @@ static void iommu_check_event_log(struct amd_iommu *iommu)
unsigned long flags;
/* RW1C interrupt status bit */
- writel(IOMMU_STATUS_EVENT_LOG_INT_MASK,
+ writel(IOMMU_STATUS_EVENT_LOG_INT,
iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
iommu_read_log(iommu, &iommu->event_log,
@@ -599,7 +598,7 @@ static void iommu_check_event_log(struct amd_iommu *iommu)
/* Check event overflow. */
entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
- if ( iommu_get_bit(entry, IOMMU_STATUS_EVENT_OVERFLOW_SHIFT) )
+ if ( entry & IOMMU_STATUS_EVENT_LOG_OVERFLOW )
iommu_reset_log(iommu, &iommu->event_log, set_iommu_event_log_control);
else
{
@@ -621,7 +620,7 @@ static void iommu_check_event_log(struct amd_iommu *iommu)
* Re-check to make sure the bit has been cleared.
*/
entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
- if ( entry & IOMMU_STATUS_EVENT_LOG_INT_MASK )
+ if ( entry & IOMMU_STATUS_EVENT_LOG_INT )
tasklet_schedule(&amd_iommu_irq_tasklet);
spin_unlock_irqrestore(&iommu->lock, flags);
@@ -678,7 +677,7 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
unsigned long flags;
/* RW1C interrupt status bit */
- writel(IOMMU_STATUS_PPR_LOG_INT_MASK,
+ writel(IOMMU_STATUS_PPR_LOG_INT,
iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
iommu_read_log(iommu, &iommu->ppr_log,
@@ -688,7 +687,7 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
/* Check event overflow. */
entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
- if ( iommu_get_bit(entry, IOMMU_STATUS_PPR_LOG_OVERFLOW_SHIFT) )
+ if ( entry & IOMMU_STATUS_PPR_LOG_OVERFLOW )
iommu_reset_log(iommu, &iommu->ppr_log, set_iommu_ppr_log_control);
else
{
@@ -710,7 +709,7 @@ static void iommu_check_ppr_log(struct amd_iommu *iommu)
* Re-check to make sure the bit has been cleared.
*/
entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
- if ( entry & IOMMU_STATUS_PPR_LOG_INT_MASK )
+ if ( entry & IOMMU_STATUS_PPR_LOG_INT )
tasklet_schedule(&amd_iommu_irq_tasklet);
spin_unlock_irqrestore(&iommu->lock, flags);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
2020-02-03 14:43 ` [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers Andrew Cooper
@ 2020-02-05 9:57 ` Jan Beulich
2020-02-10 14:39 ` Andrew Cooper
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-02-05 9:57 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 03.02.2020 15:43, Andrew Cooper wrote:
> @@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu *iommu)
> loop_count = 1000;
> do {
> status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> - comp_wait = get_field_from_reg_u32(status,
> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
> - IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
> + comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
Unless you also change comp_wait to bool, this just happens to
be correct this way because of the bit checked being at a low
enough position.
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
> BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
>
> run_bit = ( log == &iommu->event_log ) ?
> - IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
> - IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
> + IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
>
> /* wait until EventLogRun bit = 0 */
> do {
> entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
> - log_run = iommu_get_bit(entry, run_bit);
> + log_run = entry & run_bit;
Same here for log_run then. I also think run_bit would better
become unsigned int then.
With these taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
2020-02-05 9:57 ` Jan Beulich
@ 2020-02-10 14:39 ` Andrew Cooper
2020-02-10 14:59 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-10 14:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 05/02/2020 09:57, Jan Beulich wrote:
> On 03.02.2020 15:43, Andrew Cooper wrote:
>> @@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu *iommu)
>> loop_count = 1000;
>> do {
>> status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>> - comp_wait = get_field_from_reg_u32(status,
>> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
>> - IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
>> + comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
> Unless you also change comp_wait to bool, this just happens to
> be correct this way because of the bit checked being at a low
> enough position.
>
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
>> BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
>>
>> run_bit = ( log == &iommu->event_log ) ?
>> - IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
>> - IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
>> + IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
>>
>> /* wait until EventLogRun bit = 0 */
>> do {
>> entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>> - log_run = iommu_get_bit(entry, run_bit);
>> + log_run = entry & run_bit;
> Same here for log_run then. I also think run_bit would better
> become unsigned int then.
>
> With these taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
I have separate cleanup doing that (and more). I don't want to conflate
unrelated changes - this patch is complicated enough to follow.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers
2020-02-10 14:39 ` Andrew Cooper
@ 2020-02-10 14:59 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2020-02-10 14:59 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 10.02.2020 15:39, Andrew Cooper wrote:
> On 05/02/2020 09:57, Jan Beulich wrote:
>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>> @@ -85,16 +85,14 @@ static void flush_command_buffer(struct amd_iommu *iommu)
>>> loop_count = 1000;
>>> do {
>>> status = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>>> - comp_wait = get_field_from_reg_u32(status,
>>> - IOMMU_STATUS_COMP_WAIT_INT_MASK,
>>> - IOMMU_STATUS_COMP_WAIT_INT_SHIFT);
>>> + comp_wait = status & IOMMU_STATUS_COMP_WAIT_INT;
>> Unless you also change comp_wait to bool, this just happens to
>> be correct this way because of the bit checked being at a low
>> enough position.
>>
>>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>>> @@ -351,13 +351,12 @@ static void iommu_reset_log(struct amd_iommu *iommu,
>>> BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
>>>
>>> run_bit = ( log == &iommu->event_log ) ?
>>> - IOMMU_STATUS_EVENT_LOG_RUN_SHIFT :
>>> - IOMMU_STATUS_PPR_LOG_RUN_SHIFT;
>>> + IOMMU_STATUS_EVENT_LOG_RUN : IOMMU_STATUS_PPR_LOG_RUN;
>>>
>>> /* wait until EventLogRun bit = 0 */
>>> do {
>>> entry = readl(iommu->mmio_base + IOMMU_STATUS_MMIO_OFFSET);
>>> - log_run = iommu_get_bit(entry, run_bit);
>>> + log_run = entry & run_bit;
>> Same here for log_run then. I also think run_bit would better
>> become unsigned int then.
>>
>> With these taken care of
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I have separate cleanup doing that (and more). I don't want to conflate
> unrelated changes - this patch is complicated enough to follow.
But strictly speaking the assignments end up wrong this way. If
you really think such (benign) wrongness is okay, then may I
please ask that you point out this aspect in half a sentence in
the description?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Xen-devel] [PATCH 3/4] AMD/IOMMU: Treat guest head/tail pointers as byte offsets
2020-02-03 14:43 [Xen-devel] [PATCH 0/4] AMD/IOMMU: Cleanup Andrew Cooper
2020-02-03 14:43 ` [Xen-devel] [PATCH 1/4] AMD/IOMMU: Move headers to be local Andrew Cooper
2020-02-03 14:43 ` [Xen-devel] [PATCH 2/4] AMD/IOMMU: Delete iommu_{get, set, clear}_bit() helpers Andrew Cooper
@ 2020-02-03 14:43 ` Andrew Cooper
2020-02-05 10:22 ` Jan Beulich
2020-02-03 14:43 ` [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat " Andrew Cooper
3 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-03 14:43 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
The MMIO registers as already byte offsets. By masking out the reserved bits
suitably in guest_iommu_mmio_write64(), we can use the values directly,
instead of masking/shifting on every use.
Store the buffer size, rather than the number of entries, to keep the same
units for comparison purposes.
This simplifies guest_iommu_get_table_mfn() by dropping the entry_size
parameter, and simplifies the map_domain_page() handling by being able to drop
the log_base variables.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
Untested - this is all dead code, and there are plenty of security issues
which need adjusting before it can start being used again.
---
xen/drivers/passthrough/amd/iommu.h | 2 +-
xen/drivers/passthrough/amd/iommu_guest.c | 85 +++++++++++++++----------------
2 files changed, 42 insertions(+), 45 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 81b6812d3a..0b598d06f8 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -152,7 +152,7 @@ struct guest_buffer {
struct mmio_reg reg_base;
struct mmio_reg reg_tail;
struct mmio_reg reg_head;
- uint32_t entries;
+ uint32_t size;
};
struct guest_iommu_msi {
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index d05901d348..ef9c4a4d29 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -103,14 +103,13 @@ static void guest_iommu_deliver_msi(struct domain *d)
static unsigned long guest_iommu_get_table_mfn(struct domain *d,
uint64_t base_raw,
- unsigned int entry_size,
unsigned int pos)
{
unsigned long idx, gfn, mfn;
p2m_type_t p2mt;
gfn = get_gfn_from_base_reg(base_raw);
- idx = (pos * entry_size) >> PAGE_SHIFT;
+ idx = pos >> PAGE_SHIFT;
mfn = mfn_x(get_gfn(d, gfn + idx, &p2mt));
put_gfn(d, gfn);
@@ -133,14 +132,14 @@ static void guest_iommu_enable_ring_buffer(struct guest_iommu *iommu,
uint32_t length_raw = get_field_from_reg_u32(buffer->reg_base.hi,
RING_BF_LENGTH_MASK,
RING_BF_LENGTH_SHIFT);
- buffer->entries = 1 << length_raw;
+ buffer->size = entry_size << length_raw;
}
void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
{
uint16_t gdev_id;
unsigned long mfn, tail, head;
- ppr_entry_t *log, *log_base;
+ ppr_entry_t *log;
struct guest_iommu *iommu;
if ( !is_hvm_domain(d) )
@@ -150,10 +149,10 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
if ( !iommu )
return;
- tail = iommu_get_rb_pointer(iommu->ppr_log.reg_tail.lo);
- head = iommu_get_rb_pointer(iommu->ppr_log.reg_head.lo);
+ tail = iommu->ppr_log.reg_tail.lo;
+ head = iommu->ppr_log.reg_head.lo;
- if ( tail >= iommu->ppr_log.entries || head >= iommu->ppr_log.entries )
+ if ( tail >= iommu->ppr_log.size || head >= iommu->ppr_log.size )
{
AMD_IOMMU_DEBUG("Error: guest iommu ppr log overflows\n");
guest_iommu_disable(iommu);
@@ -161,11 +160,10 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
}
mfn = guest_iommu_get_table_mfn(d, reg_to_u64(iommu->ppr_log.reg_base),
- sizeof(ppr_entry_t), tail);
+ tail);
ASSERT(mfn_valid(_mfn(mfn)));
- log_base = map_domain_page(_mfn(mfn));
- log = log_base + tail % (PAGE_SIZE / sizeof(ppr_entry_t));
+ log = map_domain_page(_mfn(mfn)) + (tail & PAGE_MASK);
/* Convert physical device id back into virtual device id */
gdev_id = guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
@@ -174,13 +172,15 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
memcpy(log, entry, sizeof(ppr_entry_t));
/* Now shift ppr log tail pointer */
- if ( ++tail >= iommu->ppr_log.entries )
+ tail += sizeof(ppr_entry_t);
+ if ( tail >= iommu->ppr_log.size )
{
tail = 0;
iommu->reg_status.lo |= IOMMU_STATUS_PPR_LOG_OVERFLOW;
}
- iommu_set_rb_pointer(&iommu->ppr_log.reg_tail.lo, tail);
- unmap_domain_page(log_base);
+
+ iommu->ppr_log.reg_tail.lo = tail;
+ unmap_domain_page(log);
guest_iommu_deliver_msi(d);
}
@@ -189,7 +189,7 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
{
uint16_t dev_id;
unsigned long mfn, tail, head;
- event_entry_t *log, *log_base;
+ event_entry_t *log;
struct guest_iommu *iommu;
if ( !is_hvm_domain(d) )
@@ -199,10 +199,10 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
if ( !iommu )
return;
- tail = iommu_get_rb_pointer(iommu->event_log.reg_tail.lo);
- head = iommu_get_rb_pointer(iommu->event_log.reg_head.lo);
+ tail = iommu->event_log.reg_tail.lo;
+ head = iommu->event_log.reg_head.lo;
- if ( tail >= iommu->event_log.entries || head >= iommu->event_log.entries )
+ if ( tail >= iommu->event_log.size || head >= iommu->event_log.size )
{
AMD_IOMMU_DEBUG("Error: guest iommu event overflows\n");
guest_iommu_disable(iommu);
@@ -210,11 +210,10 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
}
mfn = guest_iommu_get_table_mfn(d, reg_to_u64(iommu->event_log.reg_base),
- sizeof(event_entry_t), tail);
+ tail);
ASSERT(mfn_valid(_mfn(mfn)));
- log_base = map_domain_page(_mfn(mfn));
- log = log_base + tail % (PAGE_SIZE / sizeof(event_entry_t));
+ log = map_domain_page(_mfn(mfn)) + (tail & PAGE_MASK);
/* re-write physical device id into virtual device id */
dev_id = guest_bdf(d, iommu_get_devid_from_cmd(entry[0]));
@@ -222,14 +221,15 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
memcpy(log, entry, sizeof(event_entry_t));
/* Now shift event log tail pointer */
- if ( ++tail >= iommu->event_log.entries )
+ tail += sizeof(event_entry_t);
+ if ( tail >= iommu->event_log.size )
{
tail = 0;
iommu->reg_status.lo |= IOMMU_STATUS_EVENT_LOG_OVERFLOW;
}
- iommu_set_rb_pointer(&iommu->event_log.reg_tail.lo, tail);
- unmap_domain_page(log_base);
+ iommu->event_log.reg_tail.lo = tail;
+ unmap_domain_page(log);
guest_iommu_deliver_msi(d);
}
@@ -379,7 +379,7 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
dte_mfn = guest_iommu_get_table_mfn(d,
reg_to_u64(g_iommu->dev_table.reg_base),
- sizeof(struct amd_iommu_dte), gbdf);
+ sizeof(struct amd_iommu_dte) * gbdf);
ASSERT(mfn_valid(_mfn(dte_mfn)));
/* Read guest dte information */
@@ -428,8 +428,8 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
static void guest_iommu_process_command(void *data)
{
- unsigned long opcode, tail, head, entries_per_page, cmd_mfn;
- cmd_entry_t *cmd, *cmd_base;
+ unsigned long opcode, tail, head, cmd_mfn;
+ cmd_entry_t *cmd;
struct domain *d = data;
struct guest_iommu *iommu;
@@ -438,34 +438,30 @@ static void guest_iommu_process_command(void *data)
if ( !iommu->enabled )
return;
- head = iommu_get_rb_pointer(iommu->cmd_buffer.reg_head.lo);
- tail = iommu_get_rb_pointer(iommu->cmd_buffer.reg_tail.lo);
+ head = iommu->cmd_buffer.reg_head.lo;
+ tail = iommu->cmd_buffer.reg_tail.lo;
/* Tail pointer is rolled over by guest driver, value outside
* cmd_buffer_entries cause iommu disabled
*/
- if ( tail >= iommu->cmd_buffer.entries ||
- head >= iommu->cmd_buffer.entries )
+ if ( tail >= iommu->cmd_buffer.size || head >= iommu->cmd_buffer.size )
{
AMD_IOMMU_DEBUG("Error: guest iommu cmd buffer overflows\n");
guest_iommu_disable(iommu);
return;
}
- entries_per_page = PAGE_SIZE / sizeof(cmd_entry_t);
-
while ( head != tail )
{
int ret = 0;
cmd_mfn = guest_iommu_get_table_mfn(d,
reg_to_u64(iommu->cmd_buffer.reg_base),
- sizeof(cmd_entry_t), head);
+ head);
ASSERT(mfn_valid(_mfn(cmd_mfn)));
- cmd_base = map_domain_page(_mfn(cmd_mfn));
- cmd = cmd_base + head % entries_per_page;
+ cmd = map_domain_page(_mfn(cmd_mfn)) + (head & PAGE_MASK);
opcode = get_field_from_reg_u32(cmd->data[1],
IOMMU_CMD_OPCODE_MASK,
@@ -498,15 +494,16 @@ static void guest_iommu_process_command(void *data)
break;
}
- unmap_domain_page(cmd_base);
- if ( ++head >= iommu->cmd_buffer.entries )
+ unmap_domain_page(cmd);
+ head += sizeof(cmd_entry_t);
+ if ( head >= iommu->cmd_buffer.size )
head = 0;
if ( ret )
guest_iommu_disable(iommu);
}
/* Now shift cmd buffer head pointer */
- iommu_set_rb_pointer(&iommu->cmd_buffer.reg_head.lo, head);
+ iommu->cmd_buffer.reg_head.lo = head;
return;
}
@@ -672,23 +669,23 @@ static void guest_iommu_mmio_write64(struct guest_iommu *iommu,
guest_iommu_write_ctrl(iommu, val);
break;
case IOMMU_CMD_BUFFER_HEAD_OFFSET:
- u64_to_reg(&iommu->cmd_buffer.reg_head, val);
+ iommu->cmd_buffer.reg_head.lo = val & IOMMU_RING_BUFFER_PTR_MASK;
break;
case IOMMU_CMD_BUFFER_TAIL_OFFSET:
- u64_to_reg(&iommu->cmd_buffer.reg_tail, val);
+ iommu->cmd_buffer.reg_tail.lo = val & IOMMU_RING_BUFFER_PTR_MASK;
tasklet_schedule(&iommu->cmd_buffer_tasklet);
break;
case IOMMU_EVENT_LOG_HEAD_OFFSET:
- u64_to_reg(&iommu->event_log.reg_head, val);
+ iommu->event_log.reg_head.lo = val & IOMMU_RING_BUFFER_PTR_MASK;
break;
case IOMMU_EVENT_LOG_TAIL_OFFSET:
- u64_to_reg(&iommu->event_log.reg_tail, val);
+ iommu->event_log.reg_tail.lo = val & IOMMU_RING_BUFFER_PTR_MASK;
break;
case IOMMU_PPR_LOG_HEAD_OFFSET:
- u64_to_reg(&iommu->ppr_log.reg_head, val);
+ iommu->ppr_log.reg_head.lo = val & IOMMU_RING_BUFFER_PTR_MASK;
break;
case IOMMU_PPR_LOG_TAIL_OFFSET:
- u64_to_reg(&iommu->ppr_log.reg_tail, val);
+ iommu->ppr_log.reg_tail.lo = val & IOMMU_RING_BUFFER_PTR_MASK;
break;
case IOMMU_STATUS_MMIO_OFFSET:
val &= IOMMU_STATUS_EVENT_LOG_OVERFLOW |
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 3/4] AMD/IOMMU: Treat guest head/tail pointers as byte offsets
2020-02-03 14:43 ` [Xen-devel] [PATCH 3/4] AMD/IOMMU: Treat guest head/tail pointers as byte offsets Andrew Cooper
@ 2020-02-05 10:22 ` Jan Beulich
2020-02-10 15:01 ` Andrew Cooper
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-02-05 10:22 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 03.02.2020 15:43, Andrew Cooper wrote:
> The MMIO registers as already byte offsets. By masking out the reserved bits
> suitably in guest_iommu_mmio_write64(), we can use the values directly,
> instead of masking/shifting on every use.
I guess it's unclear whether such masking matches real hardware
behavior, but it's certainly within spec with all other bits
there reserved.
> Store the buffer size, rather than the number of entries, to keep the same
> units for comparison purposes.
>
> This simplifies guest_iommu_get_table_mfn() by dropping the entry_size
> parameter, and simplifies the map_domain_page() handling by being able to drop
> the log_base variables.
>
> No functional change.
Well, not exactly - reads of those head/tail registers previously
returned the last written value afaict.
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] AMD/IOMMU: Treat guest head/tail pointers as byte offsets
2020-02-05 10:22 ` Jan Beulich
@ 2020-02-10 15:01 ` Andrew Cooper
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Cooper @ 2020-02-10 15:01 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 05/02/2020 10:22, Jan Beulich wrote:
> On 03.02.2020 15:43, Andrew Cooper wrote:
>> The MMIO registers as already byte offsets. By masking out the reserved bits
>> suitably in guest_iommu_mmio_write64(), we can use the values directly,
>> instead of masking/shifting on every use.
> I guess it's unclear whether such masking matches real hardware
> behavior, but it's certainly within spec with all other bits
> there reserved.
>
>> Store the buffer size, rather than the number of entries, to keep the same
>> units for comparison purposes.
>>
>> This simplifies guest_iommu_get_table_mfn() by dropping the entry_size
>> parameter, and simplifies the map_domain_page() handling by being able to drop
>> the log_base variables.
>>
>> No functional change.
> Well, not exactly - reads of those head/tail registers previously
> returned the last written value afaict.
It is actually a bugfix, because the spec prohibits preservation of
reserved bits. I'll adjust the commit message to indicate this.
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-03 14:43 [Xen-devel] [PATCH 0/4] AMD/IOMMU: Cleanup Andrew Cooper
` (2 preceding siblings ...)
2020-02-03 14:43 ` [Xen-devel] [PATCH 3/4] AMD/IOMMU: Treat guest head/tail pointers as byte offsets Andrew Cooper
@ 2020-02-03 14:43 ` Andrew Cooper
2020-02-05 10:36 ` Jan Beulich
2020-02-10 17:33 ` [Xen-devel] [PATCH v2 " Andrew Cooper
3 siblings, 2 replies; 21+ messages in thread
From: Andrew Cooper @ 2020-02-03 14:43 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
The MMIO registers as already byte offsets. Using them in this form removes
the need to shift their values for use.
It is also inefficient to store both entries and alloc_size (which differ by a
fixed entry_size). Rename alloc_size to size, and drop entries entirely,
which simplifies the allocation/deallocation helpers slightly.
Mark send_iommu_command() and invalidate_iommu_all() as static, as they have
no external declaration or callers.
Overall, this simplification allows GCC to optimise sufficiently to construct
commands directly in the command ring, rather than on the stack and copying
them into place.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
Tested on a Rome system.
---
xen/drivers/passthrough/amd/iommu-defs.h | 1 -
xen/drivers/passthrough/amd/iommu.h | 18 ++----------------
xen/drivers/passthrough/amd/iommu_cmd.c | 20 ++++++++------------
xen/drivers/passthrough/amd/iommu_init.c | 30 +++++++++++++-----------------
4 files changed, 23 insertions(+), 46 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h
index 963009de6a..50613ca150 100644
--- a/xen/drivers/passthrough/amd/iommu-defs.h
+++ b/xen/drivers/passthrough/amd/iommu-defs.h
@@ -481,7 +481,6 @@ struct amd_iommu_pte {
#define INV_IOMMU_ALL_PAGES_ADDRESS ((1ULL << 63) - 1)
#define IOMMU_RING_BUFFER_PTR_MASK 0x0007FFF0
-#define IOMMU_RING_BUFFER_PTR_SHIFT 4
#define IOMMU_CMD_DEVICE_ID_MASK 0x0000FFFF
#define IOMMU_CMD_DEVICE_ID_SHIFT 0
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 0b598d06f8..1abfdc685a 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -58,12 +58,11 @@ struct table_struct {
};
struct ring_buffer {
+ spinlock_t lock; /* protect buffer pointers */
void *buffer;
- unsigned long entries;
- unsigned long alloc_size;
uint32_t tail;
uint32_t head;
- spinlock_t lock; /* protect buffer pointers */
+ uint32_t size;
};
typedef struct iommu_cap {
@@ -379,19 +378,6 @@ static inline int iommu_has_cap(struct amd_iommu *iommu, uint32_t bit)
return !!(iommu->cap.header & (1u << bit));
}
-/* access tail or head pointer of ring buffer */
-static inline uint32_t iommu_get_rb_pointer(uint32_t reg)
-{
- return get_field_from_reg_u32(reg, IOMMU_RING_BUFFER_PTR_MASK,
- IOMMU_RING_BUFFER_PTR_SHIFT);
-}
-
-static inline void iommu_set_rb_pointer(uint32_t *reg, uint32_t val)
-{
- set_field_in_reg_u32(val, *reg, IOMMU_RING_BUFFER_PTR_MASK,
- IOMMU_RING_BUFFER_PTR_SHIFT, reg);
-}
-
/* access device id field from iommu cmd */
static inline uint16_t iommu_get_devid_from_cmd(uint32_t cmd)
{
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 166f0e7263..24ef59d436 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
{
uint32_t tail, head;
- tail = iommu->cmd_buffer.tail;
- if ( ++tail == iommu->cmd_buffer.entries )
+ tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
+ if ( tail == iommu->cmd_buffer.size )
tail = 0;
- head = iommu_get_rb_pointer(readl(iommu->mmio_base +
- IOMMU_CMD_BUFFER_HEAD_OFFSET));
+ head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
if ( head != tail )
{
- memcpy(iommu->cmd_buffer.buffer +
- (iommu->cmd_buffer.tail * IOMMU_CMD_BUFFER_ENTRY_SIZE),
+ memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail,
cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE);
iommu->cmd_buffer.tail = tail;
@@ -45,13 +43,11 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
static void commit_iommu_command_buffer(struct amd_iommu *iommu)
{
- u32 tail = 0;
-
- iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail);
- writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET);
+ writel(iommu->cmd_buffer.tail,
+ iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
}
-int send_iommu_command(struct amd_iommu *iommu, u32 cmd[])
+static int send_iommu_command(struct amd_iommu *iommu, u32 cmd[])
{
if ( queue_iommu_command(iommu, cmd) )
{
@@ -261,7 +257,7 @@ static void invalidate_interrupt_table(struct amd_iommu *iommu, u16 device_id)
send_iommu_command(iommu, cmd);
}
-void invalidate_iommu_all(struct amd_iommu *iommu)
+static void invalidate_iommu_all(struct amd_iommu *iommu)
{
u32 cmd[4], entry;
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 7bf6fef3ee..cfdeeefc2d 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -117,7 +117,7 @@ static void register_iommu_cmd_buffer_in_mmio_space(struct amd_iommu *iommu)
iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT);
writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET);
- power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.alloc_size) +
+ power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.size) +
IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE;
entry = 0;
@@ -145,7 +145,7 @@ static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu)
iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT);
writel(entry, iommu->mmio_base + IOMMU_EVENT_LOG_BASE_LOW_OFFSET);
- power_of2_entries = get_order_from_bytes(iommu->event_log.alloc_size) +
+ power_of2_entries = get_order_from_bytes(iommu->event_log.size) +
IOMMU_EVENT_LOG_POWER_OF2_ENTRIES_PER_PAGE;
entry = 0;
@@ -173,7 +173,7 @@ static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT);
writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_LOW_OFFSET);
- power_of2_entries = get_order_from_bytes(iommu->ppr_log.alloc_size) +
+ power_of2_entries = get_order_from_bytes(iommu->ppr_log.size) +
IOMMU_PPR_LOG_POWER_OF2_ENTRIES_PER_PAGE;
entry = 0;
@@ -300,7 +300,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
unsigned int entry_size,
void (*parse_func)(struct amd_iommu *, u32 *))
{
- u32 tail, head, *entry, tail_offest, head_offset;
+ u32 tail, *entry, tail_offest, head_offset;
BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
@@ -316,22 +316,20 @@ static int iommu_read_log(struct amd_iommu *iommu,
IOMMU_PPR_LOG_HEAD_OFFSET;
tail = readl(iommu->mmio_base + tail_offest);
- tail = iommu_get_rb_pointer(tail);
while ( tail != log->head )
{
/* read event log entry */
- entry = (u32 *)(log->buffer + log->head * entry_size);
+ entry = (u32 *)(log->buffer + log->head);
parse_func(iommu, entry);
- if ( ++log->head == log->entries )
+
+ log->head += entry_size;
+ if ( log->head == log->size )
log->head = 0;
/* update head pointer */
- head = 0;
- iommu_set_rb_pointer(&head, log->head);
-
- writel(head, iommu->mmio_base + head_offset);
+ writel(log->head, iommu->mmio_base + head_offset);
}
spin_unlock(&log->lock);
@@ -1001,7 +999,7 @@ static void __init deallocate_buffer(void *buf, unsigned long sz)
static void __init deallocate_ring_buffer(struct ring_buffer *ring_buf)
{
- deallocate_buffer(ring_buf->buffer, ring_buf->alloc_size);
+ deallocate_buffer(ring_buf->buffer, ring_buf->size);
ring_buf->buffer = NULL;
ring_buf->head = 0;
ring_buf->tail = 0;
@@ -1036,11 +1034,9 @@ static void *__init allocate_ring_buffer(struct ring_buffer *ring_buf,
ring_buf->tail = 0;
spin_lock_init(&ring_buf->lock);
-
- ring_buf->alloc_size = PAGE_SIZE << get_order_from_bytes(entries *
- entry_size);
- ring_buf->entries = ring_buf->alloc_size / entry_size;
- ring_buf->buffer = allocate_buffer(ring_buf->alloc_size, name, clear);
+
+ ring_buf->size = PAGE_SIZE << get_order_from_bytes(entries * entry_size);
+ ring_buf->buffer = allocate_buffer(ring_buf->size, name, clear);
return ring_buf->buffer;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-03 14:43 ` [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat " Andrew Cooper
@ 2020-02-05 10:36 ` Jan Beulich
2020-02-10 14:55 ` Andrew Cooper
2020-02-10 17:33 ` [Xen-devel] [PATCH v2 " Andrew Cooper
1 sibling, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-02-05 10:36 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 03.02.2020 15:43, Andrew Cooper wrote:
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
> {
> uint32_t tail, head;
>
> - tail = iommu->cmd_buffer.tail;
> - if ( ++tail == iommu->cmd_buffer.entries )
> + tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
> + if ( tail == iommu->cmd_buffer.size )
> tail = 0;
>
> - head = iommu_get_rb_pointer(readl(iommu->mmio_base +
> - IOMMU_CMD_BUFFER_HEAD_OFFSET));
> + head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
> if ( head != tail )
Surely you want to mask off reserved (or more generally
unrelated) bits, before consuming the value for the purpose
here (and elsewhere below)?
> @@ -45,13 +43,11 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>
> static void commit_iommu_command_buffer(struct amd_iommu *iommu)
> {
> - u32 tail = 0;
> -
> - iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail);
> - writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET);
> + writel(iommu->cmd_buffer.tail,
> + iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
I guess not preserving the reserved bits isn't a problem
right now, but is doing so a good idea in general? (I notice
the head point updating when processing the logs did so
before, so perhaps it's indeed acceptable.)
> @@ -316,22 +316,20 @@ static int iommu_read_log(struct amd_iommu *iommu,
> IOMMU_PPR_LOG_HEAD_OFFSET;
>
> tail = readl(iommu->mmio_base + tail_offest);
> - tail = iommu_get_rb_pointer(tail);
>
> while ( tail != log->head )
> {
> /* read event log entry */
> - entry = (u32 *)(log->buffer + log->head * entry_size);
> + entry = (u32 *)(log->buffer + log->head);
Would you mind dropping the pointless cast here at the same time?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-05 10:36 ` Jan Beulich
@ 2020-02-10 14:55 ` Andrew Cooper
2020-02-10 15:02 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-10 14:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 05/02/2020 10:36, Jan Beulich wrote:
> On 03.02.2020 15:43, Andrew Cooper wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>> @@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>> {
>> uint32_t tail, head;
>>
>> - tail = iommu->cmd_buffer.tail;
>> - if ( ++tail == iommu->cmd_buffer.entries )
>> + tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
>> + if ( tail == iommu->cmd_buffer.size )
>> tail = 0;
>>
>> - head = iommu_get_rb_pointer(readl(iommu->mmio_base +
>> - IOMMU_CMD_BUFFER_HEAD_OFFSET));
>> + head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>> if ( head != tail )
> Surely you want to mask off reserved (or more generally
> unrelated) bits, before consuming the value for the purpose
> here (and elsewhere below)?
Reserved bits are defined in the IOMMU spec to be read-only zero.
It is also undefined behaviour for this value to ever be outside of the
size configured for command buffer, so using the value like this is spec
compliant.
As for actually masking the values, that breaks the optimisers ability
to construct commands in the command ring. This aspect can be worked
around with other code changes, but I also think it is implausible that
the remaining reserved bits here are going to sprout incompatible future
uses.
>
>> @@ -45,13 +43,11 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>>
>> static void commit_iommu_command_buffer(struct amd_iommu *iommu)
>> {
>> - u32 tail = 0;
>> -
>> - iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail);
>> - writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET);
>> + writel(iommu->cmd_buffer.tail,
>> + iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
> I guess not preserving the reserved bits isn't a problem
> right now, but is doing so a good idea in general?
As above - there are by definition no bits to preserve.
>> @@ -316,22 +316,20 @@ static int iommu_read_log(struct amd_iommu *iommu,
>> IOMMU_PPR_LOG_HEAD_OFFSET;
>>
>> tail = readl(iommu->mmio_base + tail_offest);
>> - tail = iommu_get_rb_pointer(tail);
>>
>> while ( tail != log->head )
>> {
>> /* read event log entry */
>> - entry = (u32 *)(log->buffer + log->head * entry_size);
>> + entry = (u32 *)(log->buffer + log->head);
> Would you mind dropping the pointless cast here at the same time?
Can do.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-10 14:55 ` Andrew Cooper
@ 2020-02-10 15:02 ` Jan Beulich
2020-02-10 15:19 ` Andrew Cooper
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2020-02-10 15:02 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 10.02.2020 15:55, Andrew Cooper wrote:
> On 05/02/2020 10:36, Jan Beulich wrote:
>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>>> @@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>>> {
>>> uint32_t tail, head;
>>>
>>> - tail = iommu->cmd_buffer.tail;
>>> - if ( ++tail == iommu->cmd_buffer.entries )
>>> + tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
>>> + if ( tail == iommu->cmd_buffer.size )
>>> tail = 0;
>>>
>>> - head = iommu_get_rb_pointer(readl(iommu->mmio_base +
>>> - IOMMU_CMD_BUFFER_HEAD_OFFSET));
>>> + head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>> if ( head != tail )
>> Surely you want to mask off reserved (or more generally
>> unrelated) bits, before consuming the value for the purpose
>> here (and elsewhere below)?
>
> Reserved bits are defined in the IOMMU spec to be read-only zero.
>
> It is also undefined behaviour for this value to ever be outside of the
> size configured for command buffer, so using the value like this is spec
> compliant.
>
> As for actually masking the values, that breaks the optimisers ability
> to construct commands in the command ring. This aspect can be worked
> around with other code changes, but I also think it is implausible that
> the remaining reserved bits here are going to sprout incompatible future
> uses.
Implausible - perhaps. But impossible - no. There could be a simple
flag bit appearing somewhere in these registers. I simply don't it
is a good idea to set a precedent of not honoring reserved bit as
being reserved. The spec saying "read-only zero" can only be viewed
as correct for the current version of the spec, or else why would
the bits be called "reserved" rather than e.g. "read-as-zero"?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-10 15:02 ` Jan Beulich
@ 2020-02-10 15:19 ` Andrew Cooper
2020-02-10 16:38 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-10 15:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 10/02/2020 15:02, Jan Beulich wrote:
> On 10.02.2020 15:55, Andrew Cooper wrote:
>> On 05/02/2020 10:36, Jan Beulich wrote:
>>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>>>> @@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>>>> {
>>>> uint32_t tail, head;
>>>>
>>>> - tail = iommu->cmd_buffer.tail;
>>>> - if ( ++tail == iommu->cmd_buffer.entries )
>>>> + tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
>>>> + if ( tail == iommu->cmd_buffer.size )
>>>> tail = 0;
>>>>
>>>> - head = iommu_get_rb_pointer(readl(iommu->mmio_base +
>>>> - IOMMU_CMD_BUFFER_HEAD_OFFSET));
>>>> + head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>>> if ( head != tail )
>>> Surely you want to mask off reserved (or more generally
>>> unrelated) bits, before consuming the value for the purpose
>>> here (and elsewhere below)?
>> Reserved bits are defined in the IOMMU spec to be read-only zero.
>>
>> It is also undefined behaviour for this value to ever be outside of the
>> size configured for command buffer, so using the value like this is spec
>> compliant.
>>
>> As for actually masking the values, that breaks the optimisers ability
>> to construct commands in the command ring. This aspect can be worked
>> around with other code changes, but I also think it is implausible that
>> the remaining reserved bits here are going to sprout incompatible future
>> uses.
> Implausible - perhaps. But impossible - no. There could be a simple
> flag bit appearing somewhere in these registers. I simply don't it
> is a good idea to set a precedent of not honoring reserved bit as
> being reserved. The spec saying "read-only zero" can only be viewed
> as correct for the current version of the spec,
Its perfectly easy to do forward compatible changes with a spec written
in this way.
It means that new behaviours have to be opted in to, and this is how the
AMD IOMMU spec has evolved. Notice how every new feature declares "this
bit is reserved unless $OTHER_THING is enabled."
It is also a very sane way of doing forward compatibility, from
software's point of view.
> or else why would
> the bits be called "reserved" rather than e.g. "read-as-zero"?
Read Table 1, but it also ought to be obvious. "Reserved", "Resv" and
"Res" are all shorter to write than "read-as-zero", especially in the
diagrams of a few individual bits in a register.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-10 15:19 ` Andrew Cooper
@ 2020-02-10 16:38 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2020-02-10 16:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 10.02.2020 16:19, Andrew Cooper wrote:
> On 10/02/2020 15:02, Jan Beulich wrote:
>> On 10.02.2020 15:55, Andrew Cooper wrote:
>>> On 05/02/2020 10:36, Jan Beulich wrote:
>>>> On 03.02.2020 15:43, Andrew Cooper wrote:
>>>>> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
>>>>> @@ -24,16 +24,14 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
>>>>> {
>>>>> uint32_t tail, head;
>>>>>
>>>>> - tail = iommu->cmd_buffer.tail;
>>>>> - if ( ++tail == iommu->cmd_buffer.entries )
>>>>> + tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
>>>>> + if ( tail == iommu->cmd_buffer.size )
>>>>> tail = 0;
>>>>>
>>>>> - head = iommu_get_rb_pointer(readl(iommu->mmio_base +
>>>>> - IOMMU_CMD_BUFFER_HEAD_OFFSET));
>>>>> + head = readl(iommu->mmio_base + IOMMU_CMD_BUFFER_HEAD_OFFSET);
>>>>> if ( head != tail )
>>>> Surely you want to mask off reserved (or more generally
>>>> unrelated) bits, before consuming the value for the purpose
>>>> here (and elsewhere below)?
>>> Reserved bits are defined in the IOMMU spec to be read-only zero.
>>>
>>> It is also undefined behaviour for this value to ever be outside of the
>>> size configured for command buffer, so using the value like this is spec
>>> compliant.
>>>
>>> As for actually masking the values, that breaks the optimisers ability
>>> to construct commands in the command ring. This aspect can be worked
>>> around with other code changes, but I also think it is implausible that
>>> the remaining reserved bits here are going to sprout incompatible future
>>> uses.
>> Implausible - perhaps. But impossible - no. There could be a simple
>> flag bit appearing somewhere in these registers. I simply don't it
>> is a good idea to set a precedent of not honoring reserved bit as
>> being reserved. The spec saying "read-only zero" can only be viewed
>> as correct for the current version of the spec,
>
> Its perfectly easy to do forward compatible changes with a spec written
> in this way.
>
> It means that new behaviours have to be opted in to, and this is how the
> AMD IOMMU spec has evolved. Notice how every new feature declares "this
> bit is reserved unless $OTHER_THING is enabled."
>
> It is also a very sane way of doing forward compatibility, from
> software's point of view.
Yes. But does the IOMMU spec spell out that it'll follow this
in the future?
>> or else why would
>> the bits be called "reserved" rather than e.g. "read-as-zero"?
>
> Read Table 1, but it also ought to be obvious. "Reserved", "Resv" and
> "Res" are all shorter to write than "read-as-zero", especially in the
> diagrams of a few individual bits in a register.
There's also the common acronym "raz", which is as short. That table
in particular says nothing about future uses of currently reserved
bits. Just take the Extended Feature Register as a reference: How
would new features be advertised (in currently reserved bits) if use
of those bits was to be qualified by yet some other feature bit(s).
Past growth of the set of used bits also hasn't followed a pattern
you seem to suggest.
Don't get me wrong - I agree it's unlikely for these bits to gain
a meaning that would conflict with a more relaxed use like you do
suggest. But I don't think better code generation should be an
argument against having code written as compatibly as possible.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Xen-devel] [PATCH v2 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-03 14:43 ` [Xen-devel] [PATCH 4/4] AMD/IOMMU: Treat " Andrew Cooper
2020-02-05 10:36 ` Jan Beulich
@ 2020-02-10 17:33 ` Andrew Cooper
2020-02-11 9:23 ` Jan Beulich
1 sibling, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2020-02-10 17:33 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
The MMIO registers as already byte offsets. Using them in this form removes
the need to shift their values for use.
It is also inefficient to store both entries and alloc_size (which only differ
by entry_size). Rename alloc_size to size, and drop entries entirely, which
simplifies the allocation/deallocation helpers slightly.
Mark send_iommu_command() and invalidate_iommu_all() as static, as they have
no external declaration or callers.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
v2:
* Mask head/tail pointers
* Drop unnecessary cast.
Tested on a Rome system.
---
xen/drivers/passthrough/amd/iommu-defs.h | 1 -
xen/drivers/passthrough/amd/iommu.h | 18 ++----------------
xen/drivers/passthrough/amd/iommu_cmd.c | 21 +++++++++------------
xen/drivers/passthrough/amd/iommu_init.c | 32 ++++++++++++++------------------
4 files changed, 25 insertions(+), 47 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu-defs.h b/xen/drivers/passthrough/amd/iommu-defs.h
index 963009de6a..50613ca150 100644
--- a/xen/drivers/passthrough/amd/iommu-defs.h
+++ b/xen/drivers/passthrough/amd/iommu-defs.h
@@ -481,7 +481,6 @@ struct amd_iommu_pte {
#define INV_IOMMU_ALL_PAGES_ADDRESS ((1ULL << 63) - 1)
#define IOMMU_RING_BUFFER_PTR_MASK 0x0007FFF0
-#define IOMMU_RING_BUFFER_PTR_SHIFT 4
#define IOMMU_CMD_DEVICE_ID_MASK 0x0000FFFF
#define IOMMU_CMD_DEVICE_ID_SHIFT 0
diff --git a/xen/drivers/passthrough/amd/iommu.h b/xen/drivers/passthrough/amd/iommu.h
index 0b598d06f8..1abfdc685a 100644
--- a/xen/drivers/passthrough/amd/iommu.h
+++ b/xen/drivers/passthrough/amd/iommu.h
@@ -58,12 +58,11 @@ struct table_struct {
};
struct ring_buffer {
+ spinlock_t lock; /* protect buffer pointers */
void *buffer;
- unsigned long entries;
- unsigned long alloc_size;
uint32_t tail;
uint32_t head;
- spinlock_t lock; /* protect buffer pointers */
+ uint32_t size;
};
typedef struct iommu_cap {
@@ -379,19 +378,6 @@ static inline int iommu_has_cap(struct amd_iommu *iommu, uint32_t bit)
return !!(iommu->cap.header & (1u << bit));
}
-/* access tail or head pointer of ring buffer */
-static inline uint32_t iommu_get_rb_pointer(uint32_t reg)
-{
- return get_field_from_reg_u32(reg, IOMMU_RING_BUFFER_PTR_MASK,
- IOMMU_RING_BUFFER_PTR_SHIFT);
-}
-
-static inline void iommu_set_rb_pointer(uint32_t *reg, uint32_t val)
-{
- set_field_in_reg_u32(val, *reg, IOMMU_RING_BUFFER_PTR_MASK,
- IOMMU_RING_BUFFER_PTR_SHIFT, reg);
-}
-
/* access device id field from iommu cmd */
static inline uint16_t iommu_get_devid_from_cmd(uint32_t cmd)
{
diff --git a/xen/drivers/passthrough/amd/iommu_cmd.c b/xen/drivers/passthrough/amd/iommu_cmd.c
index 1eae339692..249ed345a0 100644
--- a/xen/drivers/passthrough/amd/iommu_cmd.c
+++ b/xen/drivers/passthrough/amd/iommu_cmd.c
@@ -24,16 +24,15 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
{
uint32_t tail, head;
- tail = iommu->cmd_buffer.tail;
- if ( ++tail == iommu->cmd_buffer.entries )
+ tail = iommu->cmd_buffer.tail + IOMMU_CMD_BUFFER_ENTRY_SIZE;
+ if ( tail == iommu->cmd_buffer.size )
tail = 0;
- head = iommu_get_rb_pointer(readl(iommu->mmio_base +
- IOMMU_CMD_BUFFER_HEAD_OFFSET));
+ head = readl(iommu->mmio_base +
+ IOMMU_CMD_BUFFER_HEAD_OFFSET) & IOMMU_RING_BUFFER_PTR_MASK;
if ( head != tail )
{
- memcpy(iommu->cmd_buffer.buffer +
- (iommu->cmd_buffer.tail * IOMMU_CMD_BUFFER_ENTRY_SIZE),
+ memcpy(iommu->cmd_buffer.buffer + iommu->cmd_buffer.tail,
cmd, IOMMU_CMD_BUFFER_ENTRY_SIZE);
iommu->cmd_buffer.tail = tail;
@@ -45,13 +44,11 @@ static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
static void commit_iommu_command_buffer(struct amd_iommu *iommu)
{
- u32 tail = 0;
-
- iommu_set_rb_pointer(&tail, iommu->cmd_buffer.tail);
- writel(tail, iommu->mmio_base+IOMMU_CMD_BUFFER_TAIL_OFFSET);
+ writel(iommu->cmd_buffer.tail,
+ iommu->mmio_base + IOMMU_CMD_BUFFER_TAIL_OFFSET);
}
-int send_iommu_command(struct amd_iommu *iommu, u32 cmd[])
+static int send_iommu_command(struct amd_iommu *iommu, u32 cmd[])
{
if ( queue_iommu_command(iommu, cmd) )
{
@@ -261,7 +258,7 @@ static void invalidate_interrupt_table(struct amd_iommu *iommu, u16 device_id)
send_iommu_command(iommu, cmd);
}
-void invalidate_iommu_all(struct amd_iommu *iommu)
+static void invalidate_iommu_all(struct amd_iommu *iommu)
{
u32 cmd[4], entry;
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index 5544dd9505..c42b608f07 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -117,7 +117,7 @@ static void register_iommu_cmd_buffer_in_mmio_space(struct amd_iommu *iommu)
iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT);
writel(entry, iommu->mmio_base + IOMMU_CMD_BUFFER_BASE_LOW_OFFSET);
- power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.alloc_size) +
+ power_of2_entries = get_order_from_bytes(iommu->cmd_buffer.size) +
IOMMU_CMD_BUFFER_POWER_OF2_ENTRIES_PER_PAGE;
entry = 0;
@@ -145,7 +145,7 @@ static void register_iommu_event_log_in_mmio_space(struct amd_iommu *iommu)
iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT);
writel(entry, iommu->mmio_base + IOMMU_EVENT_LOG_BASE_LOW_OFFSET);
- power_of2_entries = get_order_from_bytes(iommu->event_log.alloc_size) +
+ power_of2_entries = get_order_from_bytes(iommu->event_log.size) +
IOMMU_EVENT_LOG_POWER_OF2_ENTRIES_PER_PAGE;
entry = 0;
@@ -173,7 +173,7 @@ static void register_iommu_ppr_log_in_mmio_space(struct amd_iommu *iommu)
iommu_set_addr_lo_to_reg(&entry, addr_lo >> PAGE_SHIFT);
writel(entry, iommu->mmio_base + IOMMU_PPR_LOG_BASE_LOW_OFFSET);
- power_of2_entries = get_order_from_bytes(iommu->ppr_log.alloc_size) +
+ power_of2_entries = get_order_from_bytes(iommu->ppr_log.size) +
IOMMU_PPR_LOG_POWER_OF2_ENTRIES_PER_PAGE;
entry = 0;
@@ -300,7 +300,7 @@ static int iommu_read_log(struct amd_iommu *iommu,
unsigned int entry_size,
void (*parse_func)(struct amd_iommu *, u32 *))
{
- u32 tail, head, *entry, tail_offest, head_offset;
+ u32 tail, *entry, tail_offest, head_offset;
BUG_ON(!iommu || ((log != &iommu->event_log) && (log != &iommu->ppr_log)));
@@ -315,23 +315,21 @@ static int iommu_read_log(struct amd_iommu *iommu,
IOMMU_EVENT_LOG_HEAD_OFFSET :
IOMMU_PPR_LOG_HEAD_OFFSET;
- tail = readl(iommu->mmio_base + tail_offest);
- tail = iommu_get_rb_pointer(tail);
+ tail = readl(iommu->mmio_base + tail_offest) & IOMMU_RING_BUFFER_PTR_MASK;
while ( tail != log->head )
{
/* read event log entry */
- entry = (u32 *)(log->buffer + log->head * entry_size);
+ entry = log->buffer + log->head;
parse_func(iommu, entry);
- if ( ++log->head == log->entries )
+
+ log->head += entry_size;
+ if ( log->head == log->size )
log->head = 0;
/* update head pointer */
- head = 0;
- iommu_set_rb_pointer(&head, log->head);
-
- writel(head, iommu->mmio_base + head_offset);
+ writel(log->head, iommu->mmio_base + head_offset);
}
spin_unlock(&log->lock);
@@ -1000,7 +998,7 @@ static void __init deallocate_buffer(void *buf, unsigned long sz)
static void __init deallocate_ring_buffer(struct ring_buffer *ring_buf)
{
- deallocate_buffer(ring_buf->buffer, ring_buf->alloc_size);
+ deallocate_buffer(ring_buf->buffer, ring_buf->size);
ring_buf->buffer = NULL;
ring_buf->head = 0;
ring_buf->tail = 0;
@@ -1035,11 +1033,9 @@ static void *__init allocate_ring_buffer(struct ring_buffer *ring_buf,
ring_buf->tail = 0;
spin_lock_init(&ring_buf->lock);
-
- ring_buf->alloc_size = PAGE_SIZE << get_order_from_bytes(entries *
- entry_size);
- ring_buf->entries = ring_buf->alloc_size / entry_size;
- ring_buf->buffer = allocate_buffer(ring_buf->alloc_size, name, clear);
+
+ ring_buf->size = PAGE_SIZE << get_order_from_bytes(entries * entry_size);
+ ring_buf->buffer = allocate_buffer(ring_buf->size, name, clear);
return ring_buf->buffer;
}
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [Xen-devel] [PATCH v2 4/4] AMD/IOMMU: Treat head/tail pointers as byte offsets
2020-02-10 17:33 ` [Xen-devel] [PATCH v2 " Andrew Cooper
@ 2020-02-11 9:23 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2020-02-11 9:23 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 10.02.2020 18:33, Andrew Cooper wrote:
> The MMIO registers as already byte offsets. Using them in this form removes
> the need to shift their values for use.
>
> It is also inefficient to store both entries and alloc_size (which only differ
> by entry_size). Rename alloc_size to size, and drop entries entirely, which
> simplifies the allocation/deallocation helpers slightly.
>
> Mark send_iommu_command() and invalidate_iommu_all() as static, as they have
> no external declaration or callers.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> v2:
> * Mask head/tail pointers
> * Drop unnecessary cast.
Thanks for adjusting these.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread