* [Qemu-devel] [PATCH v2 01/13] iommu: Add IOMMU index concept to IOMMU API
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 18:21 ` Alex Bennée
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 02/13] iommu: Add IOMMU index argument to notifier APIs Peter Maydell
` (14 subsequent siblings)
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
If an IOMMU supports mappings that care about the memory
transaction attributes, then it no longer has a unique
address -> output mapping, but more than one. We can
represent these using an IOMMU index, analogous to TCG's
mmu indexes.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/memory.h | 55 +++++++++++++++++++++++++++++++++++++++++++
memory.c | 23 ++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index eb2ba065195..fa6e98ee7be 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr {
* to report whenever mappings are changed, by calling
* memory_region_notify_iommu() (or, if necessary, by calling
* memory_region_notify_one() for each registered notifier).
+ *
+ * Conceptually an IOMMU provides a mapping from input address
+ * to an output TLB entry. If the IOMMU is aware of memory transaction
+ * attributes and the output TLB entry depends on the transaction
+ * attributes, we represent this using IOMMU indexes. Each index
+ * selects a particular translation table that the IOMMU has:
+ * @attrs_to_index returns the IOMMU index for a set of transaction attributes
+ * @translate takes an input address and an IOMMU index
+ * and the mapping returned can only depend on the input address and the
+ * IOMMU index.
+ *
+ * Most IOMMUs don't care about the transaction attributes and support
+ * only a single IOMMU index. A more complex IOMMU might have one index
+ * for secure transactions and one for non-secure transactions.
*/
typedef struct IOMMUMemoryRegionClass {
/* private */
@@ -290,6 +304,29 @@ typedef struct IOMMUMemoryRegionClass {
*/
int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr attr,
void *data);
+
+ /* Return the IOMMU index to use for a given set of transaction attributes.
+ *
+ * Optional method: if an IOMMU only supports a single IOMMU index then
+ * the default implementation of memory_region_iommu_attrs_to_index()
+ * will return 0.
+ *
+ * The indexes supported by an IOMMU must be contiguous, starting at 0.
+ *
+ * @iommu: the IOMMUMemoryRegion
+ * @attrs: memory transaction attributes
+ */
+ int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs);
+
+ /* Return the number of IOMMU indexes this IOMMU supports.
+ *
+ * Optional method: if this method is not provided, then
+ * memory_region_iommu_num_indexes() will return 1, indicating that
+ * only a single IOMMU index is supported.
+ *
+ * @iommu: the IOMMUMemoryRegion
+ */
+ int (*num_indexes)(IOMMUMemoryRegion *iommu);
} IOMMUMemoryRegionClass;
typedef struct CoalescedMemoryRange CoalescedMemoryRange;
@@ -1054,6 +1091,24 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr,
enum IOMMUMemoryRegionAttr attr,
void *data);
+/**
+ * memory_region_iommu_attrs_to_index: return the IOMMU index to
+ * use for translations with the given memory transaction attributes.
+ *
+ * @iommu_mr: the memory region
+ * @attrs: the memory transaction attributes
+ */
+int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
+ MemTxAttrs attrs);
+
+/**
+ * memory_region_iommu_num_indexes: return the total number of IOMMU
+ * indexes that this IOMMU supports.
+ *
+ * @iommu_mr: the memory region
+ */
+int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr);
+
/**
* memory_region_name: get a memory region's name
*
diff --git a/memory.c b/memory.c
index 3212acc7f49..64f4a55d546 100644
--- a/memory.c
+++ b/memory.c
@@ -1915,6 +1915,29 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr,
return imrc->get_attr(iommu_mr, attr, data);
}
+int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
+ MemTxAttrs attrs)
+{
+ IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
+
+ if (!imrc->attrs_to_index) {
+ return 0;
+ }
+
+ return imrc->attrs_to_index(iommu_mr, attrs);
+}
+
+int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr)
+{
+ IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
+
+ if (!imrc->num_indexes) {
+ return 1;
+ }
+
+ return imrc->num_indexes(iommu_mr);
+}
+
void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
{
uint8_t mask = 1 << client;
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/13] iommu: Add IOMMU index concept to IOMMU API
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 01/13] iommu: Add IOMMU index concept to IOMMU API Peter Maydell
@ 2018-06-14 18:21 ` Alex Bennée
0 siblings, 0 replies; 40+ messages in thread
From: Alex Bennée @ 2018-06-14 18:21 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, patches, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Peter Maydell <peter.maydell@linaro.org> writes:
> If an IOMMU supports mappings that care about the memory
> transaction attributes, then it no longer has a unique
> address -> output mapping, but more than one. We can
> represent these using an IOMMU index, analogous to TCG's
> mmu indexes.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/exec/memory.h | 55 +++++++++++++++++++++++++++++++++++++++++++
> memory.c | 23 ++++++++++++++++++
> 2 files changed, 78 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index eb2ba065195..fa6e98ee7be 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr {
> * to report whenever mappings are changed, by calling
> * memory_region_notify_iommu() (or, if necessary, by calling
> * memory_region_notify_one() for each registered notifier).
> + *
> + * Conceptually an IOMMU provides a mapping from input address
> + * to an output TLB entry. If the IOMMU is aware of memory transaction
> + * attributes and the output TLB entry depends on the transaction
> + * attributes, we represent this using IOMMU indexes. Each index
> + * selects a particular translation table that the IOMMU has:
> + * @attrs_to_index returns the IOMMU index for a set of transaction attributes
> + * @translate takes an input address and an IOMMU index
> + * and the mapping returned can only depend on the input address and the
> + * IOMMU index.
> + *
> + * Most IOMMUs don't care about the transaction attributes and support
> + * only a single IOMMU index. A more complex IOMMU might have one index
> + * for secure transactions and one for non-secure transactions.
> */
> typedef struct IOMMUMemoryRegionClass {
> /* private */
> @@ -290,6 +304,29 @@ typedef struct IOMMUMemoryRegionClass {
> */
> int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr attr,
> void *data);
> +
> + /* Return the IOMMU index to use for a given set of transaction attributes.
> + *
> + * Optional method: if an IOMMU only supports a single IOMMU index then
> + * the default implementation of memory_region_iommu_attrs_to_index()
> + * will return 0.
> + *
> + * The indexes supported by an IOMMU must be contiguous, starting at 0.
> + *
> + * @iommu: the IOMMUMemoryRegion
> + * @attrs: memory transaction attributes
> + */
> + int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs);
> +
> + /* Return the number of IOMMU indexes this IOMMU supports.
> + *
> + * Optional method: if this method is not provided, then
> + * memory_region_iommu_num_indexes() will return 1, indicating that
> + * only a single IOMMU index is supported.
> + *
> + * @iommu: the IOMMUMemoryRegion
> + */
> + int (*num_indexes)(IOMMUMemoryRegion *iommu);
> } IOMMUMemoryRegionClass;
>
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> @@ -1054,6 +1091,24 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr,
> enum IOMMUMemoryRegionAttr attr,
> void *data);
>
> +/**
> + * memory_region_iommu_attrs_to_index: return the IOMMU index to
> + * use for translations with the given memory transaction attributes.
> + *
> + * @iommu_mr: the memory region
> + * @attrs: the memory transaction attributes
> + */
> +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
> + MemTxAttrs attrs);
> +
> +/**
> + * memory_region_iommu_num_indexes: return the total number of IOMMU
> + * indexes that this IOMMU supports.
> + *
> + * @iommu_mr: the memory region
> + */
> +int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr);
> +
> /**
> * memory_region_name: get a memory region's name
> *
> diff --git a/memory.c b/memory.c
> index 3212acc7f49..64f4a55d546 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1915,6 +1915,29 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr,
> return imrc->get_attr(iommu_mr, attr, data);
> }
>
> +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr,
> + MemTxAttrs attrs)
> +{
> + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
> +
> + if (!imrc->attrs_to_index) {
> + return 0;
> + }
> +
> + return imrc->attrs_to_index(iommu_mr, attrs);
> +}
> +
> +int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr)
> +{
> + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr);
> +
> + if (!imrc->num_indexes) {
> + return 1;
> + }
> +
> + return imrc->num_indexes(iommu_mr);
> +}
> +
> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> {
> uint8_t mask = 1 << client;
--
Alex Bennée
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 02/13] iommu: Add IOMMU index argument to notifier APIs
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 01/13] iommu: Add IOMMU index concept to IOMMU API Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 18:21 ` Alex Bennée
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 03/13] iommu: Add IOMMU index argument to translate method Peter Maydell
` (13 subsequent siblings)
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Add support for multiple IOMMU indexes to the IOMMU notifier APIs.
When initializing a notifier with iommu_notifier_init(), the caller
must pass the IOMMU index that it is interested in. When a change
happens, the IOMMU implementation must pass
memory_region_notify_iommu() the IOMMU index that has changed and
that notifiers must be called for.
IOMMUs which support only a single index don't need to change.
Callers which only really support working with IOMMUs with a single
index can use the result of passing MEMTXATTRS_UNSPECIFIED to
memory_region_iommu_attrs_to_index().
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/memory.h | 7 ++++++-
hw/i386/intel_iommu.c | 6 +++---
hw/ppc/spapr_iommu.c | 2 +-
hw/s390x/s390-pci-inst.c | 4 ++--
hw/vfio/common.c | 6 +++++-
hw/virtio/vhost.c | 7 ++++++-
memory.c | 8 +++++++-
7 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index fa6e98ee7be..ec75c45296e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -98,18 +98,21 @@ struct IOMMUNotifier {
/* Notify for address space range start <= addr <= end */
hwaddr start;
hwaddr end;
+ int iommu_idx;
QLIST_ENTRY(IOMMUNotifier) node;
};
typedef struct IOMMUNotifier IOMMUNotifier;
static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
IOMMUNotifierFlag flags,
- hwaddr start, hwaddr end)
+ hwaddr start, hwaddr end,
+ int iommu_idx)
{
n->notify = fn;
n->notifier_flags = flags;
n->start = start;
n->end = end;
+ n->iommu_idx = iommu_idx;
}
/*
@@ -1008,11 +1011,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr);
* should be notified with an UNMAP followed by a MAP.
*
* @iommu_mr: the memory region that was changed
+ * @iommu_idx: the IOMMU index for the translation table which has changed
* @entry: the new entry in the IOMMU translation table. The entry
* replaces all old entries for the same virtual I/O address range.
* Deleted entries have .@perm == 0.
*/
void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
+ int iommu_idx,
IOMMUTLBEntry entry);
/**
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b5a09b79089..9c0b45963b5 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -1023,7 +1023,7 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry,
void *private)
{
- memory_region_notify_iommu((IOMMUMemoryRegion *)private, *entry);
+ memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry);
return 0;
}
@@ -1581,7 +1581,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
.addr_mask = size - 1,
.perm = IOMMU_NONE,
};
- memory_region_notify_iommu(&vtd_as->iommu, entry);
+ memory_region_notify_iommu(&vtd_as->iommu, 0, entry);
}
}
}
@@ -2015,7 +2015,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
entry.iova = addr;
entry.perm = IOMMU_NONE;
entry.translated_addr = 0;
- memory_region_notify_iommu(&vtd_dev_as->iommu, entry);
+ memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry);
done:
return true;
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index aaa6010d5c9..301708e45eb 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -428,7 +428,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba,
entry.translated_addr = tce & page_mask;
entry.addr_mask = ~page_mask;
entry.perm = spapr_tce_iommu_access_flags(tce);
- memory_region_notify_iommu(&tcet->iommu, entry);
+ memory_region_notify_iommu(&tcet->iommu, 0, entry);
return H_SUCCESS;
}
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index d1a5f796783..7b61367ee31 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -589,7 +589,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry)
}
notify.perm = IOMMU_NONE;
- memory_region_notify_iommu(&iommu->iommu_mr, notify);
+ memory_region_notify_iommu(&iommu->iommu_mr, 0, notify);
notify.perm = entry->perm;
}
@@ -601,7 +601,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry)
g_hash_table_replace(iommu->iotlb, &cache->iova, cache);
}
- memory_region_notify_iommu(&iommu->iommu_mr, notify);
+ memory_region_notify_iommu(&iommu->iommu_mr, 0, notify);
}
int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8e57265edf1..fb396cf00ac 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
if (memory_region_is_iommu(section->mr)) {
VFIOGuestIOMMU *giommu;
IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
+ int iommu_idx;
trace_vfio_listener_region_add_iommu(iova, end);
/*
@@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
llend = int128_add(int128_make64(section->offset_within_region),
section->size);
llend = int128_sub(llend, int128_one());
+ iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
+ MEMTXATTRS_UNSPECIFIED);
iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
IOMMU_NOTIFIER_ALL,
section->offset_within_region,
- int128_get64(llend));
+ int128_get64(llend),
+ iommu_idx);
QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
memory_region_register_iommu_notifier(section->mr, &giommu->n);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 96175b214d7..b129cb9dddd 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -662,6 +662,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
iommu_listener);
struct vhost_iommu *iommu;
Int128 end;
+ int iommu_idx;
+ IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
if (!memory_region_is_iommu(section->mr)) {
return;
@@ -671,10 +673,13 @@ static void vhost_iommu_region_add(MemoryListener *listener,
end = int128_add(int128_make64(section->offset_within_region),
section->size);
end = int128_sub(end, int128_one());
+ iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
+ MEMTXATTRS_UNSPECIFIED);
iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
IOMMU_NOTIFIER_UNMAP,
section->offset_within_region,
- int128_get64(end));
+ int128_get64(end),
+ iommu_idx);
iommu->mr = section->mr;
iommu->iommu_offset = section->offset_within_address_space -
section->offset_within_region;
diff --git a/memory.c b/memory.c
index 64f4a55d546..7aa75ff02d3 100644
--- a/memory.c
+++ b/memory.c
@@ -1799,6 +1799,9 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr,
iommu_mr = IOMMU_MEMORY_REGION(mr);
assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
assert(n->start <= n->end);
+ assert(n->iommu_idx >= 0 &&
+ n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr));
+
QLIST_INSERT_HEAD(&iommu_mr->iommu_notify, n, node);
memory_region_update_iommu_notify_flags(iommu_mr);
}
@@ -1891,6 +1894,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
}
void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
+ int iommu_idx,
IOMMUTLBEntry entry)
{
IOMMUNotifier *iommu_notifier;
@@ -1898,7 +1902,9 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
assert(memory_region_is_iommu(MEMORY_REGION(iommu_mr)));
IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) {
- memory_region_notify_one(iommu_notifier, &entry);
+ if (iommu_notifier->iommu_idx == iommu_idx) {
+ memory_region_notify_one(iommu_notifier, &entry);
+ }
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/13] iommu: Add IOMMU index argument to notifier APIs
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 02/13] iommu: Add IOMMU index argument to notifier APIs Peter Maydell
@ 2018-06-14 18:21 ` Alex Bennée
0 siblings, 0 replies; 40+ messages in thread
From: Alex Bennée @ 2018-06-14 18:21 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, patches, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Peter Maydell <peter.maydell@linaro.org> writes:
> Add support for multiple IOMMU indexes to the IOMMU notifier APIs.
> When initializing a notifier with iommu_notifier_init(), the caller
> must pass the IOMMU index that it is interested in. When a change
> happens, the IOMMU implementation must pass
> memory_region_notify_iommu() the IOMMU index that has changed and
> that notifiers must be called for.
>
> IOMMUs which support only a single index don't need to change.
> Callers which only really support working with IOMMUs with a single
> index can use the result of passing MEMTXATTRS_UNSPECIFIED to
> memory_region_iommu_attrs_to_index().
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/exec/memory.h | 7 ++++++-
> hw/i386/intel_iommu.c | 6 +++---
> hw/ppc/spapr_iommu.c | 2 +-
> hw/s390x/s390-pci-inst.c | 4 ++--
> hw/vfio/common.c | 6 +++++-
> hw/virtio/vhost.c | 7 ++++++-
> memory.c | 8 +++++++-
> 7 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index fa6e98ee7be..ec75c45296e 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -98,18 +98,21 @@ struct IOMMUNotifier {
> /* Notify for address space range start <= addr <= end */
> hwaddr start;
> hwaddr end;
> + int iommu_idx;
> QLIST_ENTRY(IOMMUNotifier) node;
> };
> typedef struct IOMMUNotifier IOMMUNotifier;
>
> static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
> IOMMUNotifierFlag flags,
> - hwaddr start, hwaddr end)
> + hwaddr start, hwaddr end,
> + int iommu_idx)
> {
> n->notify = fn;
> n->notifier_flags = flags;
> n->start = start;
> n->end = end;
> + n->iommu_idx = iommu_idx;
> }
>
> /*
> @@ -1008,11 +1011,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr);
> * should be notified with an UNMAP followed by a MAP.
> *
> * @iommu_mr: the memory region that was changed
> + * @iommu_idx: the IOMMU index for the translation table which has changed
> * @entry: the new entry in the IOMMU translation table. The entry
> * replaces all old entries for the same virtual I/O address range.
> * Deleted entries have .@perm == 0.
> */
> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
> + int iommu_idx,
> IOMMUTLBEntry entry);
>
> /**
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index b5a09b79089..9c0b45963b5 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1023,7 +1023,7 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num,
> static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry,
> void *private)
> {
> - memory_region_notify_iommu((IOMMUMemoryRegion *)private, *entry);
> + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry);
> return 0;
> }
>
> @@ -1581,7 +1581,7 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
> .addr_mask = size - 1,
> .perm = IOMMU_NONE,
> };
> - memory_region_notify_iommu(&vtd_as->iommu, entry);
> + memory_region_notify_iommu(&vtd_as->iommu, 0, entry);
> }
> }
> }
> @@ -2015,7 +2015,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
> entry.iova = addr;
> entry.perm = IOMMU_NONE;
> entry.translated_addr = 0;
> - memory_region_notify_iommu(&vtd_dev_as->iommu, entry);
> + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry);
>
> done:
> return true;
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index aaa6010d5c9..301708e45eb 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -428,7 +428,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba,
> entry.translated_addr = tce & page_mask;
> entry.addr_mask = ~page_mask;
> entry.perm = spapr_tce_iommu_access_flags(tce);
> - memory_region_notify_iommu(&tcet->iommu, entry);
> + memory_region_notify_iommu(&tcet->iommu, 0, entry);
>
> return H_SUCCESS;
> }
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index d1a5f796783..7b61367ee31 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -589,7 +589,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry)
> }
>
> notify.perm = IOMMU_NONE;
> - memory_region_notify_iommu(&iommu->iommu_mr, notify);
> + memory_region_notify_iommu(&iommu->iommu_mr, 0, notify);
> notify.perm = entry->perm;
> }
>
> @@ -601,7 +601,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry)
> g_hash_table_replace(iommu->iotlb, &cache->iova, cache);
> }
>
> - memory_region_notify_iommu(&iommu->iommu_mr, notify);
> + memory_region_notify_iommu(&iommu->iommu_mr, 0, notify);
> }
>
> int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra)
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 8e57265edf1..fb396cf00ac 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
> if (memory_region_is_iommu(section->mr)) {
> VFIOGuestIOMMU *giommu;
> IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
> + int iommu_idx;
>
> trace_vfio_listener_region_add_iommu(iova, end);
> /*
> @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
> llend = int128_add(int128_make64(section->offset_within_region),
> section->size);
> llend = int128_sub(llend, int128_one());
> + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
> + MEMTXATTRS_UNSPECIFIED);
> iommu_notifier_init(&giommu->n, vfio_iommu_map_notify,
> IOMMU_NOTIFIER_ALL,
> section->offset_within_region,
> - int128_get64(llend));
> + int128_get64(llend),
> + iommu_idx);
> QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
>
> memory_region_register_iommu_notifier(section->mr, &giommu->n);
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 96175b214d7..b129cb9dddd 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -662,6 +662,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
> iommu_listener);
> struct vhost_iommu *iommu;
> Int128 end;
> + int iommu_idx;
> + IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>
> if (!memory_region_is_iommu(section->mr)) {
> return;
> @@ -671,10 +673,13 @@ static void vhost_iommu_region_add(MemoryListener *listener,
> end = int128_add(int128_make64(section->offset_within_region),
> section->size);
> end = int128_sub(end, int128_one());
> + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
> + MEMTXATTRS_UNSPECIFIED);
> iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify,
> IOMMU_NOTIFIER_UNMAP,
> section->offset_within_region,
> - int128_get64(end));
> + int128_get64(end),
> + iommu_idx);
> iommu->mr = section->mr;
> iommu->iommu_offset = section->offset_within_address_space -
> section->offset_within_region;
> diff --git a/memory.c b/memory.c
> index 64f4a55d546..7aa75ff02d3 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1799,6 +1799,9 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr,
> iommu_mr = IOMMU_MEMORY_REGION(mr);
> assert(n->notifier_flags != IOMMU_NOTIFIER_NONE);
> assert(n->start <= n->end);
> + assert(n->iommu_idx >= 0 &&
> + n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr));
> +
> QLIST_INSERT_HEAD(&iommu_mr->iommu_notify, n, node);
> memory_region_update_iommu_notify_flags(iommu_mr);
> }
> @@ -1891,6 +1894,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier,
> }
>
> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
> + int iommu_idx,
> IOMMUTLBEntry entry)
> {
> IOMMUNotifier *iommu_notifier;
> @@ -1898,7 +1902,9 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr,
> assert(memory_region_is_iommu(MEMORY_REGION(iommu_mr)));
>
> IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) {
> - memory_region_notify_one(iommu_notifier, &entry);
> + if (iommu_notifier->iommu_idx == iommu_idx) {
> + memory_region_notify_one(iommu_notifier, &entry);
> + }
> }
> }
--
Alex Bennée
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 03/13] iommu: Add IOMMU index argument to translate method
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 01/13] iommu: Add IOMMU index concept to IOMMU API Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 02/13] iommu: Add IOMMU index argument to notifier APIs Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 04/13] exec.c: Handle IOMMUs in address_space_translate_for_iotlb() Peter Maydell
` (12 subsequent siblings)
15 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Add an IOMMU index argument to the translate method of
IOMMUs. Since all of our current IOMMU implementations
support only a single IOMMU index, this has no effect
on the behaviour.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
include/exec/memory.h | 3 ++-
exec.c | 11 +++++++++--
hw/alpha/typhoon.c | 3 ++-
hw/arm/smmuv3.c | 2 +-
hw/dma/rc4030.c | 2 +-
hw/i386/amd_iommu.c | 2 +-
hw/i386/intel_iommu.c | 2 +-
hw/ppc/spapr_iommu.c | 3 ++-
hw/s390x/s390-pci-bus.c | 2 +-
hw/sparc/sun4m_iommu.c | 3 ++-
hw/sparc64/sun4u_iommu.c | 2 +-
memory.c | 2 +-
12 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ec75c45296e..050323f5323 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -251,9 +251,10 @@ typedef struct IOMMUMemoryRegionClass {
* @iommu: the IOMMUMemoryRegion
* @hwaddr: address to be translated within the memory region
* @flag: requested access permissions
+ * @iommu_idx: IOMMU index for the translation
*/
IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr,
- IOMMUAccessFlags flag);
+ IOMMUAccessFlags flag, int iommu_idx);
/* Returns minimum supported page size in bytes.
* If this method is not provided then the minimum is assumed to
* be TARGET_PAGE_SIZE.
diff --git a/exec.c b/exec.c
index f3fa4e9117f..033e74c36e4 100644
--- a/exec.c
+++ b/exec.c
@@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm
do {
hwaddr addr = *xlat;
IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
- IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ?
- IOMMU_WO : IOMMU_RO);
+ int iommu_idx = 0;
+ IOMMUTLBEntry iotlb;
+
+ if (imrc->attrs_to_index) {
+ iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
+ }
+
+ iotlb = imrc->translate(iommu_mr, addr, is_write ?
+ IOMMU_WO : IOMMU_RO, iommu_idx);
if (!(iotlb.perm & (1 << is_write))) {
goto unassigned;
diff --git a/hw/alpha/typhoon.c b/hw/alpha/typhoon.c
index 6a40869488e..d3ed7cdbe82 100644
--- a/hw/alpha/typhoon.c
+++ b/hw/alpha/typhoon.c
@@ -666,7 +666,8 @@ static bool window_translate(TyphoonWindow *win, hwaddr addr,
Pchip and generate a machine check interrupt. */
static IOMMUTLBEntry typhoon_translate_iommu(IOMMUMemoryRegion *iommu,
hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag,
+ int iommu_idx)
{
TyphoonPchip *pchip = container_of(iommu, TyphoonPchip, iommu);
IOMMUTLBEntry ret;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 42dc521c13a..978330900d5 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -538,7 +538,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
}
static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag, int iommu_idx)
{
SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
SMMUv3State *s = sdev->smmu;
diff --git a/hw/dma/rc4030.c b/hw/dma/rc4030.c
index 5d4833eeca3..ccd8612888e 100644
--- a/hw/dma/rc4030.c
+++ b/hw/dma/rc4030.c
@@ -491,7 +491,7 @@ static const MemoryRegionOps jazzio_ops = {
};
static IOMMUTLBEntry rc4030_dma_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag, int iommu_idx)
{
rc4030State *s = container_of(iommu, rc4030State, dma_mr);
IOMMUTLBEntry ret = {
diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 63d46ff6ee2..1fd669fef8a 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -991,7 +991,7 @@ static inline bool amdvi_is_interrupt_addr(hwaddr addr)
}
static IOMMUTLBEntry amdvi_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag, int iommu_idx)
{
AMDVIAddressSpace *as = container_of(iommu, AMDVIAddressSpace, iommu);
AMDVIState *s = as->iommu_state;
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9c0b45963b5..0a8cd4e9ccf 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2471,7 +2471,7 @@ static void vtd_mem_write(void *opaque, hwaddr addr,
}
static IOMMUTLBEntry vtd_iommu_translate(IOMMUMemoryRegion *iommu, hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag, int iommu_idx)
{
VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu);
IntelIOMMUState *s = vtd_as->iommu_state;
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 301708e45eb..1b0880ac9ed 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -112,7 +112,8 @@ static void spapr_tce_free_table(uint64_t *table, int fd, uint32_t nb_table)
/* Called from RCU critical section */
static IOMMUTLBEntry spapr_tce_translate_iommu(IOMMUMemoryRegion *iommu,
hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag,
+ int iommu_idx)
{
sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
uint64_t tce;
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 10da87458ee..e3e0ebb7f6c 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -484,7 +484,7 @@ uint16_t s390_guest_io_table_walk(uint64_t g_iota, hwaddr addr,
}
static IOMMUTLBEntry s390_translate_iommu(IOMMUMemoryRegion *mr, hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag, int iommu_idx)
{
S390PCIIOMMU *iommu = container_of(mr, S390PCIIOMMU, iommu_mr);
S390IOTLBEntry *entry;
diff --git a/hw/sparc/sun4m_iommu.c b/hw/sparc/sun4m_iommu.c
index b677601fc6b..7ca1e3fce41 100644
--- a/hw/sparc/sun4m_iommu.c
+++ b/hw/sparc/sun4m_iommu.c
@@ -282,7 +282,8 @@ static void iommu_bad_addr(IOMMUState *s, hwaddr addr,
/* Called from RCU critical section */
static IOMMUTLBEntry sun4m_translate_iommu(IOMMUMemoryRegion *iommu,
hwaddr addr,
- IOMMUAccessFlags flags)
+ IOMMUAccessFlags flags,
+ int iommu_idx)
{
IOMMUState *is = container_of(iommu, IOMMUState, iommu);
hwaddr page, pa;
diff --git a/hw/sparc64/sun4u_iommu.c b/hw/sparc64/sun4u_iommu.c
index eb3aaa87e64..1ef7645ba5f 100644
--- a/hw/sparc64/sun4u_iommu.c
+++ b/hw/sparc64/sun4u_iommu.c
@@ -73,7 +73,7 @@
/* Called from RCU critical section */
static IOMMUTLBEntry sun4u_translate_iommu(IOMMUMemoryRegion *iommu,
hwaddr addr,
- IOMMUAccessFlags flag)
+ IOMMUAccessFlags flag, int iommu_idx)
{
IOMMUState *is = container_of(iommu, IOMMUState, iommu);
hwaddr baseaddr, offset;
diff --git a/memory.c b/memory.c
index 7aa75ff02d3..21aa57d24cb 100644
--- a/memory.c
+++ b/memory.c
@@ -1832,7 +1832,7 @@ void memory_region_iommu_replay(IOMMUMemoryRegion *iommu_mr, IOMMUNotifier *n)
granularity = memory_region_iommu_get_min_page_size(iommu_mr);
for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
- iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE);
+ iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, n->iommu_idx);
if (iotlb.perm != IOMMU_NONE) {
n->notify(n, &iotlb);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [Qemu-devel] [PATCH v2 04/13] exec.c: Handle IOMMUs in address_space_translate_for_iotlb()
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (2 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 03/13] iommu: Add IOMMU index argument to translate method Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 18:23 ` Alex Bennée
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller Peter Maydell
` (11 subsequent siblings)
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Currently we don't support board configurations that put an IOMMU
in the path of the CPU's memory transactions, and instead just
assert() if the memory region fonud in address_space_translate_for_iotlb()
is an IOMMUMemoryRegion.
Remove this limitation by having the function handle IOMMUs.
This is mostly straightforward, but we must make sure we have
a notifier registered for every IOMMU that a transaction has
passed through, so that we can flush the TLB appropriately
when any of the IOMMUs change their mappings.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
include/exec/exec-all.h | 3 +-
include/qom/cpu.h | 3 +
accel/tcg/cputlb.c | 3 +-
exec.c | 135 +++++++++++++++++++++++++++++++++++++++-
4 files changed, 140 insertions(+), 4 deletions(-)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 4d09eaba72d..e0ff19b7112 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -469,7 +469,8 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
MemoryRegionSection *
address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
- hwaddr *xlat, hwaddr *plen);
+ hwaddr *xlat, hwaddr *plen,
+ MemTxAttrs attrs, int *prot);
hwaddr memory_region_section_get_iotlb(CPUState *cpu,
MemoryRegionSection *section,
target_ulong vaddr,
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 9d3afc6c759..cce2fd6acc2 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -429,6 +429,9 @@ struct CPUState {
uint16_t pending_tlb_flush;
int hvf_fd;
+
+ /* track IOMMUs whose translations we've cached in the TCG TLB */
+ GArray *iommu_notifiers;
};
QTAILQ_HEAD(CPUTailQ, CPUState);
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 05439039e91..c8acaf21e9f 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -632,7 +632,8 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
}
sz = size;
- section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz);
+ section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz,
+ attrs, &prot);
assert(sz >= TARGET_PAGE_SIZE);
tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
diff --git a/exec.c b/exec.c
index 033e74c36e4..28181115cc2 100644
--- a/exec.c
+++ b/exec.c
@@ -650,18 +650,144 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr *xlat,
return mr;
}
+typedef struct TCGIOMMUNotifier {
+ IOMMUNotifier n;
+ MemoryRegion *mr;
+ CPUState *cpu;
+ int iommu_idx;
+ bool active;
+} TCGIOMMUNotifier;
+
+static void tcg_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
+{
+ TCGIOMMUNotifier *notifier = container_of(n, TCGIOMMUNotifier, n);
+
+ if (!notifier->active) {
+ return;
+ }
+ tlb_flush(notifier->cpu);
+ notifier->active = false;
+ /* We leave the notifier struct on the list to avoid reallocating it later.
+ * Generally the number of IOMMUs a CPU deals with will be small.
+ * In any case we can't unregister the iommu notifier from a notify
+ * callback.
+ */
+}
+
+static void tcg_register_iommu_notifier(CPUState *cpu,
+ IOMMUMemoryRegion *iommu_mr,
+ int iommu_idx)
+{
+ /* Make sure this CPU has an IOMMU notifier registered for this
+ * IOMMU/IOMMU index combination, so that we can flush its TLB
+ * when the IOMMU tells us the mappings we've cached have changed.
+ */
+ MemoryRegion *mr = MEMORY_REGION(iommu_mr);
+ TCGIOMMUNotifier *notifier;
+ int i;
+
+ for (i = 0; i < cpu->iommu_notifiers->len; i++) {
+ notifier = &g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier, i);
+ if (notifier->mr == mr && notifier->iommu_idx == iommu_idx) {
+ break;
+ }
+ }
+ if (i == cpu->iommu_notifiers->len) {
+ /* Not found, add a new entry at the end of the array */
+ cpu->iommu_notifiers = g_array_set_size(cpu->iommu_notifiers, i + 1);
+ notifier = &g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier, i);
+
+ notifier->mr = mr;
+ notifier->iommu_idx = iommu_idx;
+ notifier->cpu = cpu;
+ /* Rather than trying to register interest in the specific part
+ * of the iommu's address space that we've accessed and then
+ * expand it later as subsequent accesses touch more of it, we
+ * just register interest in the whole thing, on the assumption
+ * that iommu reconfiguration will be rare.
+ */
+ iommu_notifier_init(¬ifier->n,
+ tcg_iommu_unmap_notify,
+ IOMMU_NOTIFIER_UNMAP,
+ 0,
+ HWADDR_MAX,
+ iommu_idx);
+ memory_region_register_iommu_notifier(notifier->mr, ¬ifier->n);
+ }
+
+ if (!notifier->active) {
+ notifier->active = true;
+ }
+}
+
+static void tcg_iommu_free_notifier_list(CPUState *cpu)
+{
+ /* Destroy the CPU's notifier list */
+ int i;
+ TCGIOMMUNotifier *notifier;
+
+ for (i = 0; i < cpu->iommu_notifiers->len; i++) {
+ notifier = &g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier, i);
+ memory_region_unregister_iommu_notifier(notifier->mr, ¬ifier->n);
+ }
+ g_array_free(cpu->iommu_notifiers, true);
+}
+
/* Called from RCU critical section */
MemoryRegionSection *
address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
- hwaddr *xlat, hwaddr *plen)
+ hwaddr *xlat, hwaddr *plen,
+ MemTxAttrs attrs, int *prot)
{
MemoryRegionSection *section;
+ IOMMUMemoryRegion *iommu_mr;
+ IOMMUMemoryRegionClass *imrc;
+ IOMMUTLBEntry iotlb;
+ int iommu_idx;
AddressSpaceDispatch *d = atomic_rcu_read(&cpu->cpu_ases[asidx].memory_dispatch);
- section = address_space_translate_internal(d, addr, xlat, plen, false);
+ for (;;) {
+ section = address_space_translate_internal(d, addr, &addr, plen, false);
+
+ iommu_mr = memory_region_get_iommu(section->mr);
+ if (!iommu_mr) {
+ break;
+ }
+
+ imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
+
+ iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
+ tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
+ /* We need all the permissions, so pass IOMMU_NONE so the IOMMU
+ * doesn't short-cut its translation table walk.
+ */
+ iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
+ addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
+ | (addr & iotlb.addr_mask));
+ /* Update the caller's prot bits to remove permissions the IOMMU
+ * is giving us a failure response for. If we get down to no
+ * permissions left at all we can give up now.
+ */
+ if (!(iotlb.perm & IOMMU_RO)) {
+ *prot &= ~(PAGE_READ | PAGE_EXEC);
+ }
+ if (!(iotlb.perm & IOMMU_WO)) {
+ *prot &= ~PAGE_WRITE;
+ }
+
+ if (!*prot) {
+ goto translate_fail;
+ }
+
+ d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
+ }
assert(!memory_region_is_iommu(section->mr));
+ *xlat = addr;
return section;
+
+translate_fail:
+ return &d->map.sections[PHYS_SECTION_UNASSIGNED];
}
#endif
@@ -820,6 +946,9 @@ void cpu_exec_unrealizefn(CPUState *cpu)
if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
}
+#ifndef CONFIG_USER_ONLY
+ tcg_iommu_free_notifier_list(cpu);
+#endif
}
Property cpu_common_props[] = {
@@ -867,6 +996,8 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
if (cc->vmsd != NULL) {
vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
}
+
+ cpu->iommu_notifiers = g_array_new(false, true, sizeof(TCGIOMMUNotifier));
#endif
}
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 04/13] exec.c: Handle IOMMUs in address_space_translate_for_iotlb()
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 04/13] exec.c: Handle IOMMUs in address_space_translate_for_iotlb() Peter Maydell
@ 2018-06-14 18:23 ` Alex Bennée
0 siblings, 0 replies; 40+ messages in thread
From: Alex Bennée @ 2018-06-14 18:23 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, patches, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Peter Maydell <peter.maydell@linaro.org> writes:
> Currently we don't support board configurations that put an IOMMU
> in the path of the CPU's memory transactions, and instead just
> assert() if the memory region fonud in address_space_translate_for_iotlb()
> is an IOMMUMemoryRegion.
>
> Remove this limitation by having the function handle IOMMUs.
> This is mostly straightforward, but we must make sure we have
> a notifier registered for every IOMMU that a transaction has
> passed through, so that we can flush the TLB appropriately
> when any of the IOMMUs change their mappings.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/exec/exec-all.h | 3 +-
> include/qom/cpu.h | 3 +
> accel/tcg/cputlb.c | 3 +-
> exec.c | 135 +++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 140 insertions(+), 4 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 4d09eaba72d..e0ff19b7112 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -469,7 +469,8 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
>
> MemoryRegionSection *
> address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> - hwaddr *xlat, hwaddr *plen);
> + hwaddr *xlat, hwaddr *plen,
> + MemTxAttrs attrs, int *prot);
> hwaddr memory_region_section_get_iotlb(CPUState *cpu,
> MemoryRegionSection *section,
> target_ulong vaddr,
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 9d3afc6c759..cce2fd6acc2 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -429,6 +429,9 @@ struct CPUState {
> uint16_t pending_tlb_flush;
>
> int hvf_fd;
> +
> + /* track IOMMUs whose translations we've cached in the TCG TLB */
> + GArray *iommu_notifiers;
> };
>
> QTAILQ_HEAD(CPUTailQ, CPUState);
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 05439039e91..c8acaf21e9f 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -632,7 +632,8 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
> }
>
> sz = size;
> - section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz);
> + section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz,
> + attrs, &prot);
> assert(sz >= TARGET_PAGE_SIZE);
>
> tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
> diff --git a/exec.c b/exec.c
> index 033e74c36e4..28181115cc2 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -650,18 +650,144 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr addr, hwaddr *xlat,
> return mr;
> }
>
> +typedef struct TCGIOMMUNotifier {
> + IOMMUNotifier n;
> + MemoryRegion *mr;
> + CPUState *cpu;
> + int iommu_idx;
> + bool active;
> +} TCGIOMMUNotifier;
> +
> +static void tcg_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> +{
> + TCGIOMMUNotifier *notifier = container_of(n, TCGIOMMUNotifier, n);
> +
> + if (!notifier->active) {
> + return;
> + }
> + tlb_flush(notifier->cpu);
> + notifier->active = false;
> + /* We leave the notifier struct on the list to avoid reallocating it later.
> + * Generally the number of IOMMUs a CPU deals with will be small.
> + * In any case we can't unregister the iommu notifier from a notify
> + * callback.
> + */
> +}
> +
> +static void tcg_register_iommu_notifier(CPUState *cpu,
> + IOMMUMemoryRegion *iommu_mr,
> + int iommu_idx)
> +{
> + /* Make sure this CPU has an IOMMU notifier registered for this
> + * IOMMU/IOMMU index combination, so that we can flush its TLB
> + * when the IOMMU tells us the mappings we've cached have changed.
> + */
> + MemoryRegion *mr = MEMORY_REGION(iommu_mr);
> + TCGIOMMUNotifier *notifier;
> + int i;
> +
> + for (i = 0; i < cpu->iommu_notifiers->len; i++) {
> + notifier = &g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier, i);
> + if (notifier->mr == mr && notifier->iommu_idx == iommu_idx) {
> + break;
> + }
> + }
> + if (i == cpu->iommu_notifiers->len) {
> + /* Not found, add a new entry at the end of the array */
> + cpu->iommu_notifiers = g_array_set_size(cpu->iommu_notifiers, i + 1);
> + notifier = &g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier, i);
> +
> + notifier->mr = mr;
> + notifier->iommu_idx = iommu_idx;
> + notifier->cpu = cpu;
> + /* Rather than trying to register interest in the specific part
> + * of the iommu's address space that we've accessed and then
> + * expand it later as subsequent accesses touch more of it, we
> + * just register interest in the whole thing, on the assumption
> + * that iommu reconfiguration will be rare.
> + */
> + iommu_notifier_init(¬ifier->n,
> + tcg_iommu_unmap_notify,
> + IOMMU_NOTIFIER_UNMAP,
> + 0,
> + HWADDR_MAX,
> + iommu_idx);
> + memory_region_register_iommu_notifier(notifier->mr, ¬ifier->n);
> + }
> +
> + if (!notifier->active) {
> + notifier->active = true;
> + }
> +}
> +
> +static void tcg_iommu_free_notifier_list(CPUState *cpu)
> +{
> + /* Destroy the CPU's notifier list */
> + int i;
> + TCGIOMMUNotifier *notifier;
> +
> + for (i = 0; i < cpu->iommu_notifiers->len; i++) {
> + notifier = &g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier, i);
> + memory_region_unregister_iommu_notifier(notifier->mr, ¬ifier->n);
> + }
> + g_array_free(cpu->iommu_notifiers, true);
> +}
> +
> /* Called from RCU critical section */
> MemoryRegionSection *
> address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> - hwaddr *xlat, hwaddr *plen)
> + hwaddr *xlat, hwaddr *plen,
> + MemTxAttrs attrs, int *prot)
> {
> MemoryRegionSection *section;
> + IOMMUMemoryRegion *iommu_mr;
> + IOMMUMemoryRegionClass *imrc;
> + IOMMUTLBEntry iotlb;
> + int iommu_idx;
> AddressSpaceDispatch *d = atomic_rcu_read(&cpu->cpu_ases[asidx].memory_dispatch);
>
> - section = address_space_translate_internal(d, addr, xlat, plen, false);
> + for (;;) {
> + section = address_space_translate_internal(d, addr, &addr, plen, false);
> +
> + iommu_mr = memory_region_get_iommu(section->mr);
> + if (!iommu_mr) {
> + break;
> + }
> +
> + imrc = memory_region_get_iommu_class_nocheck(iommu_mr);
> +
> + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs);
> + tcg_register_iommu_notifier(cpu, iommu_mr, iommu_idx);
> + /* We need all the permissions, so pass IOMMU_NONE so the IOMMU
> + * doesn't short-cut its translation table walk.
> + */
> + iotlb = imrc->translate(iommu_mr, addr, IOMMU_NONE, iommu_idx);
> + addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
> + | (addr & iotlb.addr_mask));
> + /* Update the caller's prot bits to remove permissions the IOMMU
> + * is giving us a failure response for. If we get down to no
> + * permissions left at all we can give up now.
> + */
> + if (!(iotlb.perm & IOMMU_RO)) {
> + *prot &= ~(PAGE_READ | PAGE_EXEC);
> + }
> + if (!(iotlb.perm & IOMMU_WO)) {
> + *prot &= ~PAGE_WRITE;
> + }
> +
> + if (!*prot) {
> + goto translate_fail;
> + }
> +
> + d = flatview_to_dispatch(address_space_to_flatview(iotlb.target_as));
> + }
>
> assert(!memory_region_is_iommu(section->mr));
> + *xlat = addr;
> return section;
> +
> +translate_fail:
> + return &d->map.sections[PHYS_SECTION_UNASSIGNED];
> }
> #endif
>
> @@ -820,6 +946,9 @@ void cpu_exec_unrealizefn(CPUState *cpu)
> if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> }
> +#ifndef CONFIG_USER_ONLY
> + tcg_iommu_free_notifier_list(cpu);
> +#endif
> }
>
> Property cpu_common_props[] = {
> @@ -867,6 +996,8 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp)
> if (cc->vmsd != NULL) {
> vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> }
> +
> + cpu->iommu_notifiers = g_array_new(false, true, sizeof(TCGIOMMUNotifier));
> #endif
> }
--
Alex Bennée
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (3 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 04/13] exec.c: Handle IOMMUs in address_space_translate_for_iotlb() Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 20:12 ` Auger Eric
2018-06-15 7:10 ` Auger Eric
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers Peter Maydell
` (10 subsequent siblings)
15 siblings, 2 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Implement the Arm TrustZone Memory Protection Controller, which sits
in front of RAM and allows secure software to configure it to either
pass through or reject transactions.
We implement the MPC as a QEMU IOMMU, which will direct transactions
either through to the devices and memory behind it or to a special
"never works" AddressSpace if they are blocked.
This initial commit implements the skeleton of the device:
* it always permits accesses
* it doesn't implement most of the registers
* it doesn't implement the interrupt or other behaviour
for blocked transactions
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
hw/misc/Makefile.objs | 1 +
include/hw/misc/tz-mpc.h | 70 ++++++
hw/misc/tz-mpc.c | 381 ++++++++++++++++++++++++++++++++
MAINTAINERS | 2 +
default-configs/arm-softmmu.mak | 1 +
hw/misc/trace-events | 7 +
6 files changed, 462 insertions(+)
create mode 100644 include/hw/misc/tz-mpc.h
create mode 100644 hw/misc/tz-mpc.c
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 00e834d0f06..7295e676a64 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -61,6 +61,7 @@ obj-$(CONFIG_MIPS_ITU) += mips_itu.o
obj-$(CONFIG_MPS2_FPGAIO) += mps2-fpgaio.o
obj-$(CONFIG_MPS2_SCC) += mps2-scc.o
+obj-$(CONFIG_TZ_MPC) += tz-mpc.o
obj-$(CONFIG_TZ_PPC) += tz-ppc.o
obj-$(CONFIG_IOTKIT_SECCTL) += iotkit-secctl.o
diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
new file mode 100644
index 00000000000..b5eaf1699ea
--- /dev/null
+++ b/include/hw/misc/tz-mpc.h
@@ -0,0 +1,70 @@
+/*
+ * ARM TrustZone memory protection controller emulation
+ *
+ * Copyright (c) 2018 Linaro Limited
+ * Written by Peter Maydell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+/* This is a model of the TrustZone memory protection controller (MPC).
+ * It is documented in the ARM CoreLink SIE-200 System IP for Embedded TRM
+ * (DDI 0571G):
+ * https://developer.arm.com/products/architecture/m-profile/docs/ddi0571/g
+ *
+ * The MPC sits in front of memory and allows secure software to
+ * configure it to either pass through or reject transactions.
+ * Rejected transactions may be configured to either be aborted, or to
+ * behave as RAZ/WI. An interrupt can be signalled for a rejected transaction.
+ *
+ * The MPC has a register interface which the guest uses to configure it.
+ *
+ * QEMU interface:
+ * + sysbus MMIO region 0: MemoryRegion for the MPC's config registers
+ * + sysbus MMIO region 1: MemoryRegion for the upstream end of the MPC
+ * + Property "downstream": MemoryRegion defining the downstream memory
+ * + Named GPIO output "irq": set for a transaction-failed interrupt
+ */
+
+#ifndef TZ_MPC_H
+#define TZ_MPC_H
+
+#include "hw/sysbus.h"
+
+#define TYPE_TZ_MPC "tz-mpc"
+#define TZ_MPC(obj) OBJECT_CHECK(TZMPC, (obj), TYPE_TZ_MPC)
+
+#define TZ_NUM_PORTS 16
+
+#define TYPE_TZ_MPC_IOMMU_MEMORY_REGION "tz-mpc-iommu-memory-region"
+
+typedef struct TZMPC TZMPC;
+
+struct TZMPC {
+ /*< private >*/
+ SysBusDevice parent_obj;
+
+ /*< public >*/
+
+ qemu_irq irq;
+
+ /* Properties */
+ MemoryRegion *downstream;
+
+ hwaddr blocksize;
+ uint32_t blk_max;
+
+ /* MemoryRegions exposed to user */
+ MemoryRegion regmr;
+ IOMMUMemoryRegion upstream;
+
+ /* MemoryRegion used internally */
+ MemoryRegion blocked_io;
+
+ AddressSpace downstream_as;
+ AddressSpace blocked_io_as;
+};
+
+#endif
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
new file mode 100644
index 00000000000..d4467ccc3b2
--- /dev/null
+++ b/hw/misc/tz-mpc.c
@@ -0,0 +1,381 @@
+/*
+ * ARM TrustZone memory protection controller emulation
+ *
+ * Copyright (c) 2018 Linaro Limited
+ * Written by Peter Maydell
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or
+ * (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "trace.h"
+#include "hw/sysbus.h"
+#include "hw/registerfields.h"
+#include "hw/misc/tz-mpc.h"
+
+/* Our IOMMU has two IOMMU indexes, one for secure transactions and one for
+ * non-secure transactions.
+ */
+enum {
+ IOMMU_IDX_S,
+ IOMMU_IDX_NS,
+ IOMMU_NUM_INDEXES,
+};
+
+/* Config registers */
+REG32(CTRL, 0x00)
+REG32(BLK_MAX, 0x10)
+REG32(BLK_CFG, 0x14)
+REG32(BLK_IDX, 0x18)
+REG32(BLK_LUT, 0x1c)
+REG32(INT_STAT, 0x20)
+REG32(INT_CLEAR, 0x24)
+REG32(INT_EN, 0x28)
+REG32(INT_INFO1, 0x2c)
+REG32(INT_INFO2, 0x30)
+REG32(INT_SET, 0x34)
+REG32(PIDR4, 0xfd0)
+REG32(PIDR5, 0xfd4)
+REG32(PIDR6, 0xfd8)
+REG32(PIDR7, 0xfdc)
+REG32(PIDR0, 0xfe0)
+REG32(PIDR1, 0xfe4)
+REG32(PIDR2, 0xfe8)
+REG32(PIDR3, 0xfec)
+REG32(CIDR0, 0xff0)
+REG32(CIDR1, 0xff4)
+REG32(CIDR2, 0xff8)
+REG32(CIDR3, 0xffc)
+
+static const uint8_t tz_mpc_idregs[] = {
+ 0x04, 0x00, 0x00, 0x00,
+ 0x60, 0xb8, 0x1b, 0x00,
+ 0x0d, 0xf0, 0x05, 0xb1,
+};
+
+static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
+ uint64_t *pdata,
+ unsigned size, MemTxAttrs attrs)
+{
+ uint64_t r;
+ uint32_t offset = addr & ~0x3;
+
+ switch (offset) {
+ case A_PIDR4:
+ case A_PIDR5:
+ case A_PIDR6:
+ case A_PIDR7:
+ case A_PIDR0:
+ case A_PIDR1:
+ case A_PIDR2:
+ case A_PIDR3:
+ case A_CIDR0:
+ case A_CIDR1:
+ case A_CIDR2:
+ case A_CIDR3:
+ r = tz_mpc_idregs[(offset - A_PIDR4) / 4];
+ break;
+ case A_INT_CLEAR:
+ case A_INT_SET:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "TZ MPC register read: write-only offset 0x%x\n",
+ offset);
+ r = 0;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "TZ MPC register read: bad offset 0x%x\n", offset);
+ r = 0;
+ break;
+ }
+
+ if (size != 4) {
+ /* None of our registers are read-sensitive (except BLK_LUT,
+ * which can special case the "size not 4" case), so just
+ * pull the right bytes out of the word read result.
+ */
+ r = extract32(r, (addr & 3) * 8, size * 8);
+ }
+
+ trace_tz_mpc_reg_read(addr, r, size);
+ *pdata = r;
+ return MEMTX_OK;
+}
+
+static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
+ uint64_t value,
+ unsigned size, MemTxAttrs attrs)
+{
+ uint32_t offset = addr & ~0x3;
+
+ trace_tz_mpc_reg_write(addr, value, size);
+
+ if (size != 4) {
+ /* Expand the byte or halfword write to a full word size.
+ * In most cases we can do this with zeroes; the exceptions
+ * are CTRL, BLK_IDX and BLK_LUT.
+ */
+ uint32_t oldval;
+
+ switch (offset) {
+ /* As we add support for registers which need expansions
+ * other than zeroes we'll fill in cases here.
+ */
+ default:
+ oldval = 0;
+ break;
+ }
+ value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
+ }
+
+ switch (offset) {
+ case A_PIDR4:
+ case A_PIDR5:
+ case A_PIDR6:
+ case A_PIDR7:
+ case A_PIDR0:
+ case A_PIDR1:
+ case A_PIDR2:
+ case A_PIDR3:
+ case A_CIDR0:
+ case A_CIDR1:
+ case A_CIDR2:
+ case A_CIDR3:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "TZ MPC register write: read-only offset 0x%x\n", offset);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "TZ MPC register write: bad offset 0x%x\n", offset);
+ break;
+ }
+
+ return MEMTX_OK;
+}
+
+static const MemoryRegionOps tz_mpc_reg_ops = {
+ .read_with_attrs = tz_mpc_reg_read,
+ .write_with_attrs = tz_mpc_reg_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 4,
+ .impl.min_access_size = 1,
+ .impl.max_access_size = 4,
+};
+
+/* Accesses only reach these read and write functions if the MPC is
+ * blocking them; non-blocked accesses go directly to the downstream
+ * memory region without passing through this code.
+ */
+static MemTxResult tz_mpc_mem_blocked_read(void *opaque, hwaddr addr,
+ uint64_t *pdata,
+ unsigned size, MemTxAttrs attrs)
+{
+ trace_tz_mpc_mem_blocked_read(addr, size, attrs.secure);
+
+ *pdata = 0;
+ return MEMTX_OK;
+}
+
+static MemTxResult tz_mpc_mem_blocked_write(void *opaque, hwaddr addr,
+ uint64_t value,
+ unsigned size, MemTxAttrs attrs)
+{
+ trace_tz_mpc_mem_blocked_write(addr, value, size, attrs.secure);
+
+ return MEMTX_OK;
+}
+
+static const MemoryRegionOps tz_mpc_mem_blocked_ops = {
+ .read_with_attrs = tz_mpc_mem_blocked_read,
+ .write_with_attrs = tz_mpc_mem_blocked_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid.min_access_size = 1,
+ .valid.max_access_size = 8,
+ .impl.min_access_size = 1,
+ .impl.max_access_size = 8,
+};
+
+static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
+ hwaddr addr, IOMMUAccessFlags flags,
+ int iommu_idx)
+{
+ TZMPC *s = TZ_MPC(container_of(iommu, TZMPC, upstream));
+ bool ok;
+
+ IOMMUTLBEntry ret = {
+ .iova = addr & ~(s->blocksize - 1),
+ .translated_addr = addr & ~(s->blocksize - 1),
+ .addr_mask = s->blocksize - 1,
+ .perm = IOMMU_RW,
+ };
+
+ /* Look at the per-block configuration for this address, and
+ * return a TLB entry directing the transaction at either
+ * downstream_as or blocked_io_as, as appropriate.
+ * For the moment, always permit accesses.
+ */
+ ok = true;
+
+ trace_tz_mpc_translate(addr, flags,
+ iommu_idx == IOMMU_IDX_S ? "S" : "NS",
+ ok ? "pass" : "block");
+
+ ret.target_as = ok ? &s->downstream_as : &s->blocked_io_as;
+ return ret;
+}
+
+static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
+{
+ /* We treat unspecified attributes like secure. Transactions with
+ * unspecified attributes come from places like
+ * cpu_physical_memory_write_rom() for initial image load, and we want
+ * those to pass through the from-reset "everything is secure" config.
+ * All the real during-emulation transactions from the CPU will
+ * specify attributes.
+ */
+ return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
+}
+
+static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
+{
+ return IOMMU_NUM_INDEXES;
+}
+
+static void tz_mpc_reset(DeviceState *dev)
+{
+}
+
+static void tz_mpc_init(Object *obj)
+{
+ DeviceState *dev = DEVICE(obj);
+ TZMPC *s = TZ_MPC(obj);
+
+ qdev_init_gpio_out_named(dev, &s->irq, "irq", 1);
+}
+
+static void tz_mpc_realize(DeviceState *dev, Error **errp)
+{
+ Object *obj = OBJECT(dev);
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+ TZMPC *s = TZ_MPC(dev);
+ uint64_t size;
+
+ /* We can't create the upstream end of the port until realize,
+ * as we don't know the size of the MR used as the downstream until then.
+ * We insist on having a downstream, to avoid complicating the code
+ * with handling the "don't know how big this is" case. It's easy
+ * enough for the user to create an unimplemented_device as downstream
+ * if they have nothing else to plug into this.
+ */
+ if (!s->downstream) {
+ error_setg(errp, "MPC 'downstream' link not set");
+ return;
+ }
+
+ size = memory_region_size(s->downstream);
+
+ memory_region_init_iommu(&s->upstream, sizeof(s->upstream),
+ TYPE_TZ_MPC_IOMMU_MEMORY_REGION,
+ obj, "tz-mpc-upstream", size);
+
+ /* In real hardware the block size is configurable. In QEMU we could
+ * make it configurable but will need it to be at least as big as the
+ * target page size so we can execute out of the resulting MRs. Guest
+ * software is supposed to check the block size using the BLK_CFG
+ * register, so make it fixed at the page size.
+ */
+ s->blocksize = memory_region_iommu_get_min_page_size(&s->upstream);
+ if (size % s->blocksize != 0) {
+ error_setg(errp,
+ "MPC 'downstream' size %" PRId64
+ " is not a multiple of %" HWADDR_PRIx " bytes",
+ size, s->blocksize);
+ object_unref(OBJECT(&s->upstream));
+ return;
+ }
+
+ /* BLK_MAX is the max value of BLK_IDX, which indexes an array of 32-bit
+ * words, each bit of which indicates one block.
+ */
+ s->blk_max = DIV_ROUND_UP(size / s->blocksize, 32);
+
+ memory_region_init_io(&s->regmr, obj, &tz_mpc_reg_ops,
+ s, "tz-mpc-regs", 0x1000);
+ sysbus_init_mmio(sbd, &s->regmr);
+
+ sysbus_init_mmio(sbd, MEMORY_REGION(&s->upstream));
+
+ /* This memory region is not exposed to users of this device as a
+ * sysbus MMIO region, but is instead used internally as something
+ * that our IOMMU translate function might direct accesses to.
+ */
+ memory_region_init_io(&s->blocked_io, obj, &tz_mpc_mem_blocked_ops,
+ s, "tz-mpc-blocked-io", size);
+
+ address_space_init(&s->downstream_as, s->downstream,
+ "tz-mpc-downstream");
+ address_space_init(&s->blocked_io_as, &s->blocked_io,
+ "tz-mpc-blocked-io");
+}
+
+static const VMStateDescription tz_mpc_vmstate = {
+ .name = "tz-mpc",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static Property tz_mpc_properties[] = {
+ DEFINE_PROP_LINK("downstream", TZMPC, downstream,
+ TYPE_MEMORY_REGION, MemoryRegion *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tz_mpc_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = tz_mpc_realize;
+ dc->vmsd = &tz_mpc_vmstate;
+ dc->reset = tz_mpc_reset;
+ dc->props = tz_mpc_properties;
+}
+
+static const TypeInfo tz_mpc_info = {
+ .name = TYPE_TZ_MPC,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(TZMPC),
+ .instance_init = tz_mpc_init,
+ .class_init = tz_mpc_class_init,
+};
+
+static void tz_mpc_iommu_memory_region_class_init(ObjectClass *klass,
+ void *data)
+{
+ IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
+
+ imrc->translate = tz_mpc_translate;
+ imrc->attrs_to_index = tz_mpc_attrs_to_index;
+ imrc->num_indexes = tz_mpc_num_indexes;
+}
+
+static const TypeInfo tz_mpc_iommu_memory_region_info = {
+ .name = TYPE_TZ_MPC_IOMMU_MEMORY_REGION,
+ .parent = TYPE_IOMMU_MEMORY_REGION,
+ .class_init = tz_mpc_iommu_memory_region_class_init,
+};
+
+static void tz_mpc_register_types(void)
+{
+ type_register_static(&tz_mpc_info);
+ type_register_static(&tz_mpc_iommu_memory_region_info);
+}
+
+type_init(tz_mpc_register_types);
diff --git a/MAINTAINERS b/MAINTAINERS
index 41cd3736a9b..9b712b92021 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -449,6 +449,8 @@ F: hw/char/cmsdk-apb-uart.c
F: include/hw/char/cmsdk-apb-uart.h
F: hw/misc/tz-ppc.c
F: include/hw/misc/tz-ppc.h
+F: hw/misc/tz-mpc.c
+F: include/hw/misc/tz-mpc.h
ARM cores
M: Peter Maydell <peter.maydell@linaro.org>
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 8ba2558b36a..0f7dc2eb315 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -107,6 +107,7 @@ CONFIG_CMSDK_APB_UART=y
CONFIG_MPS2_FPGAIO=y
CONFIG_MPS2_SCC=y
+CONFIG_TZ_MPC=y
CONFIG_TZ_PPC=y
CONFIG_IOTKIT=y
CONFIG_IOTKIT_SECCTL=y
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index ec5a9f0da13..72bf9d57000 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -84,6 +84,13 @@ mos6522_set_sr_int(void) "set sr_int"
mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
+# hw/misc/tz-mpc.c
+tz_mpc_reg_read(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs read: offset 0x%x data 0x%" PRIx64 " size %u"
+tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs write: offset 0x%x data 0x%" PRIx64 " size %u"
+tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"
+tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"
+tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"
+
# hw/misc/tz-ppc.c
tz_ppc_reset(void) "TZ PPC: reset"
tz_ppc_cfg_nonsec(int n, int level) "TZ PPC: cfg_nonsec[%d] = %d"
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller Peter Maydell
@ 2018-06-14 20:12 ` Auger Eric
2018-06-15 7:10 ` Auger Eric
1 sibling, 0 replies; 40+ messages in thread
From: Auger Eric @ 2018-06-14 20:12 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini, Peter Xu
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the Arm TrustZone Memory Protection Controller, which sits
> in front of RAM and allows secure software to configure it to either
> pass through or reject transactions.
>
> We implement the MPC as a QEMU IOMMU, which will direct transactions
> either through to the devices and memory behind it or to a special
> "never works" AddressSpace if they are blocked.
>
> This initial commit implements the skeleton of the device:
> * it always permits accesses
> * it doesn't implement most of the registers
> * it doesn't implement the interrupt or other behaviour
> for blocked transactions
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> hw/misc/Makefile.objs | 1 +
> include/hw/misc/tz-mpc.h | 70 ++++++
> hw/misc/tz-mpc.c | 381 ++++++++++++++++++++++++++++++++
> MAINTAINERS | 2 +
> default-configs/arm-softmmu.mak | 1 +
> hw/misc/trace-events | 7 +
> 6 files changed, 462 insertions(+)
> create mode 100644 include/hw/misc/tz-mpc.h
> create mode 100644 hw/misc/tz-mpc.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 00e834d0f06..7295e676a64 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -61,6 +61,7 @@ obj-$(CONFIG_MIPS_ITU) += mips_itu.o
> obj-$(CONFIG_MPS2_FPGAIO) += mps2-fpgaio.o
> obj-$(CONFIG_MPS2_SCC) += mps2-scc.o
>
> +obj-$(CONFIG_TZ_MPC) += tz-mpc.o
> obj-$(CONFIG_TZ_PPC) += tz-ppc.o
> obj-$(CONFIG_IOTKIT_SECCTL) += iotkit-secctl.o
>
> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
> new file mode 100644
> index 00000000000..b5eaf1699ea
> --- /dev/null
> +++ b/include/hw/misc/tz-mpc.h
> @@ -0,0 +1,70 @@
> +/*
> + * ARM TrustZone memory protection controller emulation
nit AHB5 TrustZone Memory Protection controller? This is the title of
chapter in the spec.
> + *
> + * Copyright (c) 2018 Linaro Limited
> + * Written by Peter Maydell
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +/* This is a model of the TrustZone memory protection controller (MPC).
> + * It is documented in the ARM CoreLink SIE-200 System IP for Embedded TRM
> + * (DDI 0571G):
> + * https://developer.arm.com/products/architecture/m-profile/docs/ddi0571/g
> + *
> + * The MPC sits in front of memory and allows secure software to
> + * configure it to either pass through or reject transactions.
> + * Rejected transactions may be configured to either be aborted, or to
> + * behave as RAZ/WI. An interrupt can be signalled for a rejected transaction.
> + *
> + * The MPC has a register interface which the guest uses to configure it.
> + *
> + * QEMU interface:
> + * + sysbus MMIO region 0: MemoryRegion for the MPC's config registers
> + * + sysbus MMIO region 1: MemoryRegion for the upstream end of the MPC
> + * + Property "downstream": MemoryRegion defining the downstream memory
> + * + Named GPIO output "irq": set for a transaction-failed interrupt
> + */
> +
> +#ifndef TZ_MPC_H
> +#define TZ_MPC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_TZ_MPC "tz-mpc"
> +#define TZ_MPC(obj) OBJECT_CHECK(TZMPC, (obj), TYPE_TZ_MPC)
> +
> +#define TZ_NUM_PORTS 16
> +
> +#define TYPE_TZ_MPC_IOMMU_MEMORY_REGION "tz-mpc-iommu-memory-region"
> +
> +typedef struct TZMPC TZMPC;
> +
> +struct TZMPC {
> + /*< private >*/
> + SysBusDevice parent_obj;
> +
> + /*< public >*/
> +
> + qemu_irq irq;
> +
> + /* Properties */
> + MemoryRegion *downstream;
> +
> + hwaddr blocksize;
> + uint32_t blk_max;
> +
> + /* MemoryRegions exposed to user */
> + MemoryRegion regmr;
> + IOMMUMemoryRegion upstream;
> +
> + /* MemoryRegion used internally */
> + MemoryRegion blocked_io;
> +
> + AddressSpace downstream_as;
> + AddressSpace blocked_io_as;
> +};
> +
> +#endif
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> new file mode 100644
> index 00000000000..d4467ccc3b2
> --- /dev/null
> +++ b/hw/misc/tz-mpc.c
> @@ -0,0 +1,381 @@
> +/*
> + * ARM TrustZone memory protection controller emulation
> + *
> + * Copyright (c) 2018 Linaro Limited
> + * Written by Peter Maydell
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +#include "hw/sysbus.h"
> +#include "hw/registerfields.h"
> +#include "hw/misc/tz-mpc.h"
> +
> +/* Our IOMMU has two IOMMU indexes, one for secure transactions and one for
> + * non-secure transactions.
> + */
> +enum {
> + IOMMU_IDX_S,
> + IOMMU_IDX_NS,
> + IOMMU_NUM_INDEXES,
> +};
> +
> +/* Config registers */
> +REG32(CTRL, 0x00)
> +REG32(BLK_MAX, 0x10)
> +REG32(BLK_CFG, 0x14)
> +REG32(BLK_IDX, 0x18)
> +REG32(BLK_LUT, 0x1c)
> +REG32(INT_STAT, 0x20)
> +REG32(INT_CLEAR, 0x24)
> +REG32(INT_EN, 0x28)
> +REG32(INT_INFO1, 0x2c)
> +REG32(INT_INFO2, 0x30)
> +REG32(INT_SET, 0x34)
> +REG32(PIDR4, 0xfd0)
> +REG32(PIDR5, 0xfd4)
> +REG32(PIDR6, 0xfd8)
> +REG32(PIDR7, 0xfdc)
> +REG32(PIDR0, 0xfe0)
> +REG32(PIDR1, 0xfe4)
> +REG32(PIDR2, 0xfe8)
> +REG32(PIDR3, 0xfec)
> +REG32(CIDR0, 0xff0)
> +REG32(CIDR1, 0xff4)
> +REG32(CIDR2, 0xff8)
> +REG32(CIDR3, 0xffc)
> +
> +static const uint8_t tz_mpc_idregs[] = {
> + 0x04, 0x00, 0x00, 0x00,
> + 0x60, 0xb8, 0x1b, 0x00,
> + 0x0d, 0xf0, 0x05, 0xb1,
> +};
> +
> +static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
> + uint64_t *pdata,
> + unsigned size, MemTxAttrs attrs)
> +{
> + uint64_t r;
> + uint32_t offset = addr & ~0x3;
> +
> + switch (offset) {
> + case A_PIDR4:
> + case A_PIDR5:
> + case A_PIDR6:
> + case A_PIDR7:
> + case A_PIDR0:
> + case A_PIDR1:
> + case A_PIDR2:
> + case A_PIDR3:
> + case A_CIDR0:
> + case A_CIDR1:
> + case A_CIDR2:
> + case A_CIDR3:
> + r = tz_mpc_idregs[(offset - A_PIDR4) / 4];
> + break;
> + case A_INT_CLEAR:
> + case A_INT_SET:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register read: write-only offset 0x%x\n",
> + offset);
> + r = 0;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register read: bad offset 0x%x\n", offset);
> + r = 0;
> + break;
> + }
> +
> + if (size != 4) {
> + /* None of our registers are read-sensitive (except BLK_LUT,
> + * which can special case the "size not 4" case), so just
> + * pull the right bytes out of the word read result.
> + */
> + r = extract32(r, (addr & 3) * 8, size * 8);
> + }
> +
> + trace_tz_mpc_reg_read(addr, r, size);
> + *pdata = r;
> + return MEMTX_OK;
> +}
> +
> +static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> + uint64_t value,
> + unsigned size, MemTxAttrs attrs)
> +{
> + uint32_t offset = addr & ~0x3;
> +
> + trace_tz_mpc_reg_write(addr, value, size);
> +
> + if (size != 4) {
> + /* Expand the byte or halfword write to a full word size.
> + * In most cases we can do this with zeroes; the exceptions
> + * are CTRL, BLK_IDX and BLK_LUT.
> + */
> + uint32_t oldval;
> +
> + switch (offset) {
> + /* As we add support for registers which need expansions
> + * other than zeroes we'll fill in cases here.
> + */
> + default:
> + oldval = 0;
> + break;
> + }
> + value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
> + }
> +
> + switch (offset) {
> + case A_PIDR4:
> + case A_PIDR5:
> + case A_PIDR6:
> + case A_PIDR7:
> + case A_PIDR0:
> + case A_PIDR1:
> + case A_PIDR2:
> + case A_PIDR3:
> + case A_CIDR0:
> + case A_CIDR1:
> + case A_CIDR2:
> + case A_CIDR3:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register write: read-only offset 0x%x\n", offset);
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register write: bad offset 0x%x\n", offset);
> + break;
> + }
> +
> + return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps tz_mpc_reg_ops = {
> + .read_with_attrs = tz_mpc_reg_read,
> + .write_with_attrs = tz_mpc_reg_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 4,
> + .impl.min_access_size = 1,
> + .impl.max_access_size = 4,
> +};
> +
> +/* Accesses only reach these read and write functions if the MPC is
> + * blocking them; non-blocked accesses go directly to the downstream
> + * memory region without passing through this code.
> + */
> +static MemTxResult tz_mpc_mem_blocked_read(void *opaque, hwaddr addr,
> + uint64_t *pdata,
> + unsigned size, MemTxAttrs attrs)
> +{
> + trace_tz_mpc_mem_blocked_read(addr, size, attrs.secure);
> +
> + *pdata = 0;
> + return MEMTX_OK;
> +}
> +
> +static MemTxResult tz_mpc_mem_blocked_write(void *opaque, hwaddr addr,
> + uint64_t value,
> + unsigned size, MemTxAttrs attrs)
> +{
> + trace_tz_mpc_mem_blocked_write(addr, value, size, attrs.secure);
> +
> + return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps tz_mpc_mem_blocked_ops = {
> + .read_with_attrs = tz_mpc_mem_blocked_read,
> + .write_with_attrs = tz_mpc_mem_blocked_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 8,
> + .impl.min_access_size = 1,
> + .impl.max_access_size = 8,
> +};
> +
> +static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
> + hwaddr addr, IOMMUAccessFlags flags,
> + int iommu_idx)
> +{
> + TZMPC *s = TZ_MPC(container_of(iommu, TZMPC, upstream));
> + bool ok;
> +
> + IOMMUTLBEntry ret = {
> + .iova = addr & ~(s->blocksize - 1),
> + .translated_addr = addr & ~(s->blocksize - 1),
> + .addr_mask = s->blocksize - 1,
> + .perm = IOMMU_RW,
> + };
> +
> + /* Look at the per-block configuration for this address, and
> + * return a TLB entry directing the transaction at either
> + * downstream_as or blocked_io_as, as appropriate.
> + * For the moment, always permit accesses.
> + */
> + ok = true;
> +
> + trace_tz_mpc_translate(addr, flags,
> + iommu_idx == IOMMU_IDX_S ? "S" : "NS",
> + ok ? "pass" : "block");
> +
> + ret.target_as = ok ? &s->downstream_as : &s->blocked_io_as;
> + return ret;
> +}
> +
> +static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
> +{
> + /* We treat unspecified attributes like secure. Transactions with
> + * unspecified attributes come from places like
> + * cpu_physical_memory_write_rom() for initial image load, and we want
> + * those to pass through the from-reset "everything is secure" config.
> + * All the real during-emulation transactions from the CPU will
> + * specify attributes.
> + */
> + return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
> +}
> +
> +static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
> +{
> + return IOMMU_NUM_INDEXES;
> +}
> +
> +static void tz_mpc_reset(DeviceState *dev)
> +{
> +}
> +
> +static void tz_mpc_init(Object *obj)
> +{
> + DeviceState *dev = DEVICE(obj);
> + TZMPC *s = TZ_MPC(obj);
> +
> + qdev_init_gpio_out_named(dev, &s->irq, "irq", 1);
> +}
> +
> +static void tz_mpc_realize(DeviceState *dev, Error **errp)
> +{
> + Object *obj = OBJECT(dev);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + TZMPC *s = TZ_MPC(dev);
> + uint64_t size;
> +
> + /* We can't create the upstream end of the port until realize,
> + * as we don't know the size of the MR used as the downstream until then.
> + * We insist on having a downstream, to avoid complicating the code
> + * with handling the "don't know how big this is" case. It's easy
> + * enough for the user to create an unimplemented_device as downstream
> + * if they have nothing else to plug into this.
> + */
> + if (!s->downstream) {
> + error_setg(errp, "MPC 'downstream' link not set");
> + return;
> + }
> +
> + size = memory_region_size(s->downstream);
> +
> + memory_region_init_iommu(&s->upstream, sizeof(s->upstream),
> + TYPE_TZ_MPC_IOMMU_MEMORY_REGION,
> + obj, "tz-mpc-upstream", size);
> +
> + /* In real hardware the block size is configurable. In QEMU we could
> + * make it configurable but will need it to be at least as big as the
> + * target page size so we can execute out of the resulting MRs. Guest
> + * software is supposed to check the block size using the BLK_CFG
> + * register, so make it fixed at the page size.
> + */
> + s->blocksize = memory_region_iommu_get_min_page_size(&s->upstream);
> + if (size % s->blocksize != 0) {
> + error_setg(errp,
> + "MPC 'downstream' size %" PRId64
> + " is not a multiple of %" HWADDR_PRIx " bytes",
> + size, s->blocksize);
> + object_unref(OBJECT(&s->upstream));
> + return;
> + }
> +
> + /* BLK_MAX is the max value of BLK_IDX, which indexes an array of 32-bit
> + * words, each bit of which indicates one block.
> + */
> + s->blk_max = DIV_ROUND_UP(size / s->blocksize, 32);
> +
> + memory_region_init_io(&s->regmr, obj, &tz_mpc_reg_ops,
> + s, "tz-mpc-regs", 0x1000);
> + sysbus_init_mmio(sbd, &s->regmr);
> +
> + sysbus_init_mmio(sbd, MEMORY_REGION(&s->upstream));
> +
> + /* This memory region is not exposed to users of this device as a
> + * sysbus MMIO region, but is instead used internally as something
> + * that our IOMMU translate function might direct accesses to.
> + */
> + memory_region_init_io(&s->blocked_io, obj, &tz_mpc_mem_blocked_ops,
> + s, "tz-mpc-blocked-io", size);
> +
> + address_space_init(&s->downstream_as, s->downstream,
> + "tz-mpc-downstream");
> + address_space_init(&s->blocked_io_as, &s->blocked_io,
> + "tz-mpc-blocked-io");
> +}
> +
> +static const VMStateDescription tz_mpc_vmstate = {
> + .name = "tz-mpc",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static Property tz_mpc_properties[] = {
> + DEFINE_PROP_LINK("downstream", TZMPC, downstream,
> + TYPE_MEMORY_REGION, MemoryRegion *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tz_mpc_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = tz_mpc_realize;
> + dc->vmsd = &tz_mpc_vmstate;
> + dc->reset = tz_mpc_reset;
> + dc->props = tz_mpc_properties;
> +}
> +
> +static const TypeInfo tz_mpc_info = {
> + .name = TYPE_TZ_MPC,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(TZMPC),
> + .instance_init = tz_mpc_init,
> + .class_init = tz_mpc_class_init,
> +};
> +
> +static void tz_mpc_iommu_memory_region_class_init(ObjectClass *klass,
> + void *data)
> +{
> + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
> +
> + imrc->translate = tz_mpc_translate;
> + imrc->attrs_to_index = tz_mpc_attrs_to_index;
> + imrc->num_indexes = tz_mpc_num_indexes;
> +}
> +
> +static const TypeInfo tz_mpc_iommu_memory_region_info = {
> + .name = TYPE_TZ_MPC_IOMMU_MEMORY_REGION,
> + .parent = TYPE_IOMMU_MEMORY_REGION,
> + .class_init = tz_mpc_iommu_memory_region_class_init,
> +};
> +
> +static void tz_mpc_register_types(void)
> +{
> + type_register_static(&tz_mpc_info);
> + type_register_static(&tz_mpc_iommu_memory_region_info);
> +}
> +
> +type_init(tz_mpc_register_types);
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41cd3736a9b..9b712b92021 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -449,6 +449,8 @@ F: hw/char/cmsdk-apb-uart.c
> F: include/hw/char/cmsdk-apb-uart.h
> F: hw/misc/tz-ppc.c
> F: include/hw/misc/tz-ppc.h
> +F: hw/misc/tz-mpc.c
> +F: include/hw/misc/tz-mpc.h
>
> ARM cores
> M: Peter Maydell <peter.maydell@linaro.org>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 8ba2558b36a..0f7dc2eb315 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -107,6 +107,7 @@ CONFIG_CMSDK_APB_UART=y
> CONFIG_MPS2_FPGAIO=y
> CONFIG_MPS2_SCC=y
>
> +CONFIG_TZ_MPC=y
> CONFIG_TZ_PPC=y
> CONFIG_IOTKIT=y
> CONFIG_IOTKIT_SECCTL=y
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index ec5a9f0da13..72bf9d57000 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -84,6 +84,13 @@ mos6522_set_sr_int(void) "set sr_int"
> mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
> mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
>
> +# hw/misc/tz-mpc.c
> +tz_mpc_reg_read(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs read: offset 0x%x data 0x%" PRIx64 " size %u"
> +tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs write: offset 0x%x data 0x%" PRIx64 " size %u"
> +tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"
> +tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"
> +tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"
> +
> # hw/misc/tz-ppc.c
> tz_ppc_reset(void) "TZ PPC: reset"
> tz_ppc_cfg_nonsec(int n, int level) "TZ PPC: cfg_nonsec[%d] = %d"
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller Peter Maydell
2018-06-14 20:12 ` Auger Eric
@ 2018-06-15 7:10 ` Auger Eric
2018-06-15 8:53 ` Peter Maydell
1 sibling, 1 reply; 40+ messages in thread
From: Auger Eric @ 2018-06-15 7:10 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Peter Xu, Paolo Bonzini, Alex Bennée, Richard Henderson
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the Arm TrustZone Memory Protection Controller, which sits
> in front of RAM and allows secure software to configure it to either
> pass through or reject transactions.
>
> We implement the MPC as a QEMU IOMMU, which will direct transactions
> either through to the devices and memory behind it or to a special
> "never works" AddressSpace if they are blocked.
>
> This initial commit implements the skeleton of the device:
> * it always permits accesses
> * it doesn't implement most of the registers
> * it doesn't implement the interrupt or other behaviour
> for blocked transactions
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> hw/misc/Makefile.objs | 1 +
> include/hw/misc/tz-mpc.h | 70 ++++++
> hw/misc/tz-mpc.c | 381 ++++++++++++++++++++++++++++++++
> MAINTAINERS | 2 +
> default-configs/arm-softmmu.mak | 1 +
> hw/misc/trace-events | 7 +
> 6 files changed, 462 insertions(+)
> create mode 100644 include/hw/misc/tz-mpc.h
> create mode 100644 hw/misc/tz-mpc.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 00e834d0f06..7295e676a64 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -61,6 +61,7 @@ obj-$(CONFIG_MIPS_ITU) += mips_itu.o
> obj-$(CONFIG_MPS2_FPGAIO) += mps2-fpgaio.o
> obj-$(CONFIG_MPS2_SCC) += mps2-scc.o
>
> +obj-$(CONFIG_TZ_MPC) += tz-mpc.o
> obj-$(CONFIG_TZ_PPC) += tz-ppc.o
> obj-$(CONFIG_IOTKIT_SECCTL) += iotkit-secctl.o
>
> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
> new file mode 100644
> index 00000000000..b5eaf1699ea
> --- /dev/null
> +++ b/include/hw/misc/tz-mpc.h
> @@ -0,0 +1,70 @@
> +/*
> + * ARM TrustZone memory protection controller emulation
> + *
> + * Copyright (c) 2018 Linaro Limited
> + * Written by Peter Maydell
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +/* This is a model of the TrustZone memory protection controller (MPC).
> + * It is documented in the ARM CoreLink SIE-200 System IP for Embedded TRM
> + * (DDI 0571G):
> + * https://developer.arm.com/products/architecture/m-profile/docs/ddi0571/g
> + *
> + * The MPC sits in front of memory and allows secure software to
> + * configure it to either pass through or reject transactions.
> + * Rejected transactions may be configured to either be aborted, or to
> + * behave as RAZ/WI. An interrupt can be signalled for a rejected transaction.
> + *
> + * The MPC has a register interface which the guest uses to configure it.
> + *
> + * QEMU interface:
> + * + sysbus MMIO region 0: MemoryRegion for the MPC's config registers
> + * + sysbus MMIO region 1: MemoryRegion for the upstream end of the MPC
> + * + Property "downstream": MemoryRegion defining the downstream memory
> + * + Named GPIO output "irq": set for a transaction-failed interrupt
> + */
> +
> +#ifndef TZ_MPC_H
> +#define TZ_MPC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_TZ_MPC "tz-mpc"
> +#define TZ_MPC(obj) OBJECT_CHECK(TZMPC, (obj), TYPE_TZ_MPC)
> +
> +#define TZ_NUM_PORTS 16
> +
> +#define TYPE_TZ_MPC_IOMMU_MEMORY_REGION "tz-mpc-iommu-memory-region"
> +
> +typedef struct TZMPC TZMPC;
> +
> +struct TZMPC {
> + /*< private >*/
> + SysBusDevice parent_obj;
> +
> + /*< public >*/
> +
> + qemu_irq irq;
> +
> + /* Properties */
> + MemoryRegion *downstream;
> +
> + hwaddr blocksize;
> + uint32_t blk_max;
> +
> + /* MemoryRegions exposed to user */
> + MemoryRegion regmr;
> + IOMMUMemoryRegion upstream;
> +
> + /* MemoryRegion used internally */
> + MemoryRegion blocked_io;
> +
> + AddressSpace downstream_as;
> + AddressSpace blocked_io_as;
> +};
> +
> +#endif
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> new file mode 100644
> index 00000000000..d4467ccc3b2
> --- /dev/null
> +++ b/hw/misc/tz-mpc.c
> @@ -0,0 +1,381 @@
> +/*
> + * ARM TrustZone memory protection controller emulation
> + *
> + * Copyright (c) 2018 Linaro Limited
> + * Written by Peter Maydell
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or
> + * (at your option) any later version.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "trace.h"
> +#include "hw/sysbus.h"
> +#include "hw/registerfields.h"
> +#include "hw/misc/tz-mpc.h"
> +
> +/* Our IOMMU has two IOMMU indexes, one for secure transactions and one for
> + * non-secure transactions.
> + */
> +enum {
> + IOMMU_IDX_S,
> + IOMMU_IDX_NS,
> + IOMMU_NUM_INDEXES,
> +};
> +
> +/* Config registers */
> +REG32(CTRL, 0x00)
> +REG32(BLK_MAX, 0x10)
> +REG32(BLK_CFG, 0x14)
> +REG32(BLK_IDX, 0x18)
> +REG32(BLK_LUT, 0x1c)
> +REG32(INT_STAT, 0x20)
> +REG32(INT_CLEAR, 0x24)
> +REG32(INT_EN, 0x28)
> +REG32(INT_INFO1, 0x2c)
> +REG32(INT_INFO2, 0x30)
> +REG32(INT_SET, 0x34)
> +REG32(PIDR4, 0xfd0)
> +REG32(PIDR5, 0xfd4)
> +REG32(PIDR6, 0xfd8)
> +REG32(PIDR7, 0xfdc)
> +REG32(PIDR0, 0xfe0)
> +REG32(PIDR1, 0xfe4)
> +REG32(PIDR2, 0xfe8)
> +REG32(PIDR3, 0xfec)
> +REG32(CIDR0, 0xff0)
> +REG32(CIDR1, 0xff4)
> +REG32(CIDR2, 0xff8)
> +REG32(CIDR3, 0xffc)
> +
> +static const uint8_t tz_mpc_idregs[] = {
> + 0x04, 0x00, 0x00, 0x00,
> + 0x60, 0xb8, 0x1b, 0x00,
> + 0x0d, 0xf0, 0x05, 0xb1,
> +};
> +
> +static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
> + uint64_t *pdata,
> + unsigned size, MemTxAttrs attrs)
> +{
> + uint64_t r;
> + uint32_t offset = addr & ~0x3;
> +
> + switch (offset) {
> + case A_PIDR4:
> + case A_PIDR5:
> + case A_PIDR6:
> + case A_PIDR7:
> + case A_PIDR0:
> + case A_PIDR1:
> + case A_PIDR2:
> + case A_PIDR3:
> + case A_CIDR0:
> + case A_CIDR1:
> + case A_CIDR2:
> + case A_CIDR3:
> + r = tz_mpc_idregs[(offset - A_PIDR4) / 4];
> + break;
> + case A_INT_CLEAR:
> + case A_INT_SET:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register read: write-only offset 0x%x\n",
> + offset);
> + r = 0;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register read: bad offset 0x%x\n", offset);
> + r = 0;
> + break;
> + }
> +
> + if (size != 4) {
> + /* None of our registers are read-sensitive (except BLK_LUT,
> + * which can special case the "size not 4" case), so just
> + * pull the right bytes out of the word read result.
> + */
> + r = extract32(r, (addr & 3) * 8, size * 8);
> + }
> +
> + trace_tz_mpc_reg_read(addr, r, size);
> + *pdata = r;
> + return MEMTX_OK;
> +}
> +
> +static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> + uint64_t value,
> + unsigned size, MemTxAttrs attrs)
> +{
> + uint32_t offset = addr & ~0x3;
> +
> + trace_tz_mpc_reg_write(addr, value, size);
> +
> + if (size != 4) {
> + /* Expand the byte or halfword write to a full word size.
> + * In most cases we can do this with zeroes; the exceptions
> + * are CTRL, BLK_IDX and BLK_LUT.
> + */
> + uint32_t oldval;
> +
> + switch (offset) {
> + /* As we add support for registers which need expansions
> + * other than zeroes we'll fill in cases here.
> + */
> + default:
> + oldval = 0;
> + break;
> + }
> + value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
> + }
> +
> + switch (offset) {
> + case A_PIDR4:
> + case A_PIDR5:
> + case A_PIDR6:
> + case A_PIDR7:
> + case A_PIDR0:
> + case A_PIDR1:
> + case A_PIDR2:
> + case A_PIDR3:
> + case A_CIDR0:
> + case A_CIDR1:
> + case A_CIDR2:
> + case A_CIDR3:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register write: read-only offset 0x%x\n", offset);
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "TZ MPC register write: bad offset 0x%x\n", offset);
> + break;
> + }
> +
> + return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps tz_mpc_reg_ops = {
> + .read_with_attrs = tz_mpc_reg_read,
> + .write_with_attrs = tz_mpc_reg_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 4,
> + .impl.min_access_size = 1,
> + .impl.max_access_size = 4,
> +};
> +
> +/* Accesses only reach these read and write functions if the MPC is
> + * blocking them; non-blocked accesses go directly to the downstream
> + * memory region without passing through this code.
> + */
> +static MemTxResult tz_mpc_mem_blocked_read(void *opaque, hwaddr addr,
> + uint64_t *pdata,
> + unsigned size, MemTxAttrs attrs)
> +{
> + trace_tz_mpc_mem_blocked_read(addr, size, attrs.secure);
> +
> + *pdata = 0;
> + return MEMTX_OK;
> +}
> +
> +static MemTxResult tz_mpc_mem_blocked_write(void *opaque, hwaddr addr,
> + uint64_t value,
> + unsigned size, MemTxAttrs attrs)
> +{
> + trace_tz_mpc_mem_blocked_write(addr, value, size, attrs.secure);
> +
> + return MEMTX_OK;
> +}
> +
> +static const MemoryRegionOps tz_mpc_mem_blocked_ops = {
> + .read_with_attrs = tz_mpc_mem_blocked_read,
> + .write_with_attrs = tz_mpc_mem_blocked_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid.min_access_size = 1,
> + .valid.max_access_size = 8,
> + .impl.min_access_size = 1,
> + .impl.max_access_size = 8,
> +};
> +
> +static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
> + hwaddr addr, IOMMUAccessFlags flags,
> + int iommu_idx)
> +{
> + TZMPC *s = TZ_MPC(container_of(iommu, TZMPC, upstream));
> + bool ok;
> +
> + IOMMUTLBEntry ret = {
> + .iova = addr & ~(s->blocksize - 1),
> + .translated_addr = addr & ~(s->blocksize - 1),
> + .addr_mask = s->blocksize - 1,
> + .perm = IOMMU_RW,
> + };
> +
> + /* Look at the per-block configuration for this address, and
> + * return a TLB entry directing the transaction at either
> + * downstream_as or blocked_io_as, as appropriate.
> + * For the moment, always permit accesses.
> + */
> + ok = true;
> +
> + trace_tz_mpc_translate(addr, flags,
> + iommu_idx == IOMMU_IDX_S ? "S" : "NS",
> + ok ? "pass" : "block");
> +
> + ret.target_as = ok ? &s->downstream_as : &s->blocked_io_as;
> + return ret;
after reading 8/13, I have a doubt here about the ret.perm value that
stays IOMMU_RW independently on the translation success. Usually if the
translation failn perm is set to IOMMU_NONE. In your case you also play
with the output address space which switches to the blocked one in case
of failure and you handle access failure through that means. So maybe
the end result is the same but I am not sure.
Thanks
Eric
> +}
> +
> +static int tz_mpc_attrs_to_index(IOMMUMemoryRegion *iommu, MemTxAttrs attrs)
> +{
> + /* We treat unspecified attributes like secure. Transactions with
> + * unspecified attributes come from places like
> + * cpu_physical_memory_write_rom() for initial image load, and we want
> + * those to pass through the from-reset "everything is secure" config.
> + * All the real during-emulation transactions from the CPU will
> + * specify attributes.
> + */
> + return (attrs.unspecified || attrs.secure) ? IOMMU_IDX_S : IOMMU_IDX_NS;
> +}
> +
> +static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
> +{
> + return IOMMU_NUM_INDEXES;
> +}
> +
> +static void tz_mpc_reset(DeviceState *dev)
> +{
> +}
> +
> +static void tz_mpc_init(Object *obj)
> +{
> + DeviceState *dev = DEVICE(obj);
> + TZMPC *s = TZ_MPC(obj);
> +
> + qdev_init_gpio_out_named(dev, &s->irq, "irq", 1);
> +}
> +
> +static void tz_mpc_realize(DeviceState *dev, Error **errp)
> +{
> + Object *obj = OBJECT(dev);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> + TZMPC *s = TZ_MPC(dev);
> + uint64_t size;
> +
> + /* We can't create the upstream end of the port until realize,
> + * as we don't know the size of the MR used as the downstream until then.
> + * We insist on having a downstream, to avoid complicating the code
> + * with handling the "don't know how big this is" case. It's easy
> + * enough for the user to create an unimplemented_device as downstream
> + * if they have nothing else to plug into this.
> + */
> + if (!s->downstream) {
> + error_setg(errp, "MPC 'downstream' link not set");
> + return;
> + }
> +
> + size = memory_region_size(s->downstream);
> +
> + memory_region_init_iommu(&s->upstream, sizeof(s->upstream),
> + TYPE_TZ_MPC_IOMMU_MEMORY_REGION,
> + obj, "tz-mpc-upstream", size);
> +
> + /* In real hardware the block size is configurable. In QEMU we could
> + * make it configurable but will need it to be at least as big as the
> + * target page size so we can execute out of the resulting MRs. Guest
> + * software is supposed to check the block size using the BLK_CFG
> + * register, so make it fixed at the page size.
> + */
> + s->blocksize = memory_region_iommu_get_min_page_size(&s->upstream);
> + if (size % s->blocksize != 0) {
> + error_setg(errp,
> + "MPC 'downstream' size %" PRId64
> + " is not a multiple of %" HWADDR_PRIx " bytes",
> + size, s->blocksize);
> + object_unref(OBJECT(&s->upstream));
> + return;
> + }
> +
> + /* BLK_MAX is the max value of BLK_IDX, which indexes an array of 32-bit
> + * words, each bit of which indicates one block.
> + */
> + s->blk_max = DIV_ROUND_UP(size / s->blocksize, 32);
> +
> + memory_region_init_io(&s->regmr, obj, &tz_mpc_reg_ops,
> + s, "tz-mpc-regs", 0x1000);
> + sysbus_init_mmio(sbd, &s->regmr);
> +
> + sysbus_init_mmio(sbd, MEMORY_REGION(&s->upstream));
> +
> + /* This memory region is not exposed to users of this device as a
> + * sysbus MMIO region, but is instead used internally as something
> + * that our IOMMU translate function might direct accesses to.
> + */
> + memory_region_init_io(&s->blocked_io, obj, &tz_mpc_mem_blocked_ops,
> + s, "tz-mpc-blocked-io", size);
> +
> + address_space_init(&s->downstream_as, s->downstream,
> + "tz-mpc-downstream");
> + address_space_init(&s->blocked_io_as, &s->blocked_io,
> + "tz-mpc-blocked-io");
> +}
> +
> +static const VMStateDescription tz_mpc_vmstate = {
> + .name = "tz-mpc",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static Property tz_mpc_properties[] = {
> + DEFINE_PROP_LINK("downstream", TZMPC, downstream,
> + TYPE_MEMORY_REGION, MemoryRegion *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tz_mpc_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = tz_mpc_realize;
> + dc->vmsd = &tz_mpc_vmstate;
> + dc->reset = tz_mpc_reset;
> + dc->props = tz_mpc_properties;
> +}
> +
> +static const TypeInfo tz_mpc_info = {
> + .name = TYPE_TZ_MPC,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(TZMPC),
> + .instance_init = tz_mpc_init,
> + .class_init = tz_mpc_class_init,
> +};
> +
> +static void tz_mpc_iommu_memory_region_class_init(ObjectClass *klass,
> + void *data)
> +{
> + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
> +
> + imrc->translate = tz_mpc_translate;
> + imrc->attrs_to_index = tz_mpc_attrs_to_index;
> + imrc->num_indexes = tz_mpc_num_indexes;
> +}
> +
> +static const TypeInfo tz_mpc_iommu_memory_region_info = {
> + .name = TYPE_TZ_MPC_IOMMU_MEMORY_REGION,
> + .parent = TYPE_IOMMU_MEMORY_REGION,
> + .class_init = tz_mpc_iommu_memory_region_class_init,
> +};
> +
> +static void tz_mpc_register_types(void)
> +{
> + type_register_static(&tz_mpc_info);
> + type_register_static(&tz_mpc_iommu_memory_region_info);
> +}
> +
> +type_init(tz_mpc_register_types);
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 41cd3736a9b..9b712b92021 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -449,6 +449,8 @@ F: hw/char/cmsdk-apb-uart.c
> F: include/hw/char/cmsdk-apb-uart.h
> F: hw/misc/tz-ppc.c
> F: include/hw/misc/tz-ppc.h
> +F: hw/misc/tz-mpc.c
> +F: include/hw/misc/tz-mpc.h
>
> ARM cores
> M: Peter Maydell <peter.maydell@linaro.org>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 8ba2558b36a..0f7dc2eb315 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -107,6 +107,7 @@ CONFIG_CMSDK_APB_UART=y
> CONFIG_MPS2_FPGAIO=y
> CONFIG_MPS2_SCC=y
>
> +CONFIG_TZ_MPC=y
> CONFIG_TZ_PPC=y
> CONFIG_IOTKIT=y
> CONFIG_IOTKIT_SECCTL=y
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index ec5a9f0da13..72bf9d57000 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -84,6 +84,13 @@ mos6522_set_sr_int(void) "set sr_int"
> mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
> mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
>
> +# hw/misc/tz-mpc.c
> +tz_mpc_reg_read(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs read: offset 0x%x data 0x%" PRIx64 " size %u"
> +tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs write: offset 0x%x data 0x%" PRIx64 " size %u"
> +tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"
> +tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"
> +tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"
> +
> # hw/misc/tz-ppc.c
> tz_ppc_reset(void) "TZ PPC: reset"
> tz_ppc_cfg_nonsec(int n, int level) "TZ PPC: cfg_nonsec[%d] = %d"
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
2018-06-15 7:10 ` Auger Eric
@ 2018-06-15 8:53 ` Peter Maydell
2018-06-15 13:23 ` Auger Eric
0 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 8:53 UTC (permalink / raw)
To: Auger Eric
Cc: qemu-arm, QEMU Developers, patches, Peter Xu, Paolo Bonzini,
Alex Bennée, Richard Henderson
On 15 June 2018 at 08:10, Auger Eric <eric.auger@redhat.com> wrote:
> after reading 8/13, I have a doubt here about the ret.perm value that
> stays IOMMU_RW independently on the translation success. Usually if the
> translation failn perm is set to IOMMU_NONE. In your case you also play
> with the output address space which switches to the blocked one in case
> of failure and you handle access failure through that means. So maybe
> the end result is the same but I am not sure.
The end result is different, which is why we have to return RW here.
If we return NONE we are telling the caller that this will be
a memory transaction failure, so the CPU emulation will generate
a bus-error kind of exception. By returning RW we tell the caller
that it should make transactions for this address. We can then
handle them in the MPC with the correct behaviour for a blocked
access (generating an interrupt, and configurably either completing
the access as RAZ/WI or returning MEMTX_ERROR to cause a busfault.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
2018-06-15 8:53 ` Peter Maydell
@ 2018-06-15 13:23 ` Auger Eric
0 siblings, 0 replies; 40+ messages in thread
From: Auger Eric @ 2018-06-15 13:23 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, QEMU Developers, patches, Peter Xu, Paolo Bonzini,
Alex Bennée, Richard Henderson
Hi Peter,
On 06/15/2018 10:53 AM, Peter Maydell wrote:
> On 15 June 2018 at 08:10, Auger Eric <eric.auger@redhat.com> wrote:
>> after reading 8/13, I have a doubt here about the ret.perm value that
>> stays IOMMU_RW independently on the translation success. Usually if the
>> translation failn perm is set to IOMMU_NONE. In your case you also play
>> with the output address space which switches to the blocked one in case
>> of failure and you handle access failure through that means. So maybe
>> the end result is the same but I am not sure.
>
> The end result is different, which is why we have to return RW here.
> If we return NONE we are telling the caller that this will be
> a memory transaction failure, so the CPU emulation will generate
> a bus-error kind of exception. By returning RW we tell the caller
> that it should make transactions for this address. We can then
> handle them in the MPC with the correct behaviour for a blocked
> access (generating an interrupt, and configurably either completing
> the access as RAZ/WI or returning MEMTX_ERROR to cause a busfault.)
OK thank you for the explanation
Eric
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (4 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 05/13] hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 20:14 ` Auger Eric
` (2 more replies)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 07/13] hw/misc/tz-mpc.c: Implement correct blocked-access behaviour Peter Maydell
` (9 subsequent siblings)
15 siblings, 3 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Implement the missing registers for the TZ MPC.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
include/hw/misc/tz-mpc.h | 10 +++
hw/misc/tz-mpc.c | 137 ++++++++++++++++++++++++++++++++++++++-
2 files changed, 144 insertions(+), 3 deletions(-)
diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
index b5eaf1699ea..1fff4d6029a 100644
--- a/include/hw/misc/tz-mpc.h
+++ b/include/hw/misc/tz-mpc.h
@@ -48,6 +48,16 @@ struct TZMPC {
/*< public >*/
+ /* State */
+ uint32_t ctrl;
+ uint32_t blk_idx;
+ uint32_t int_stat;
+ uint32_t int_en;
+ uint32_t int_info1;
+ uint32_t int_info2;
+
+ uint32_t *blk_lut;
+
qemu_irq irq;
/* Properties */
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index d4467ccc3b2..9db55e23daf 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -28,16 +28,23 @@ enum {
/* Config registers */
REG32(CTRL, 0x00)
+ FIELD(CTRL, SEC_RESP, 4, 1)
+ FIELD(CTRL, AUTOINC, 8, 1)
+ FIELD(CTRL, LOCKDOWN, 31, 1)
REG32(BLK_MAX, 0x10)
REG32(BLK_CFG, 0x14)
REG32(BLK_IDX, 0x18)
REG32(BLK_LUT, 0x1c)
REG32(INT_STAT, 0x20)
+ FIELD(INT_STAT, IRQ, 0, 1)
REG32(INT_CLEAR, 0x24)
+ FIELD(INT_CLEAR, IRQ, 0, 1)
REG32(INT_EN, 0x28)
+ FIELD(INT_EN, IRQ, 0, 1)
REG32(INT_INFO1, 0x2c)
REG32(INT_INFO2, 0x30)
REG32(INT_SET, 0x34)
+ FIELD(INT_SET, IRQ, 0, 1)
REG32(PIDR4, 0xfd0)
REG32(PIDR5, 0xfd4)
REG32(PIDR6, 0xfd8)
@@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {
0x0d, 0xf0, 0x05, 0xb1,
};
+static void tz_mpc_irq_update(TZMPC *s)
+{
+ qemu_set_irq(s->irq, s->int_stat && s->int_en);
+}
+
static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
uint64_t *pdata,
unsigned size, MemTxAttrs attrs)
{
+ TZMPC *s = TZ_MPC(opaque);
uint64_t r;
uint32_t offset = addr & ~0x3;
switch (offset) {
+ case A_CTRL:
+ r = s->ctrl;
+ break;
+ case A_BLK_MAX:
+ r = s->blk_max;
+ break;
+ case A_BLK_CFG:
+ /* We are never in "init in progress state", so this just indicates
+ * the block size. s->blocksize == (1 << BLK_CFG + 5), so
+ * BLK_CFG == ctz32(s->blocksize) - 5
+ */
+ r = ctz32(s->blocksize) - 5;
+ break;
+ case A_BLK_IDX:
+ r = s->blk_idx;
+ break;
+ case A_BLK_LUT:
+ r = s->blk_lut[s->blk_idx];
+ if (size == 4) {
+ s->blk_idx++;
+ s->blk_idx %= s->blk_max;
+ }
+ break;
+ case A_INT_STAT:
+ r = s->int_stat;
+ break;
+ case A_INT_EN:
+ r = s->int_en;
+ break;
+ case A_INT_INFO1:
+ r = s->int_info1;
+ break;
+ case A_INT_INFO2:
+ r = s->int_info2;
+ break;
case A_PIDR4:
case A_PIDR5:
case A_PIDR6:
@@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
uint64_t value,
unsigned size, MemTxAttrs attrs)
{
+ TZMPC *s = TZ_MPC(opaque);
uint32_t offset = addr & ~0x3;
trace_tz_mpc_reg_write(addr, value, size);
@@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
uint32_t oldval;
switch (offset) {
- /* As we add support for registers which need expansions
- * other than zeroes we'll fill in cases here.
- */
+ case A_CTRL:
+ oldval = s->ctrl;
+ break;
+ case A_BLK_IDX:
+ oldval = s->blk_idx;
+ break;
+ case A_BLK_LUT:
+ oldval = s->blk_lut[s->blk_idx];
+ break;
default:
oldval = 0;
break;
@@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
}
+ if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&
+ (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {
+ /* Lockdown mode makes these three registers read-only, and
+ * the only way out of it is to reset the device.
+ */
+ qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "
+ "while MPC is in lockdown mode\n", offset);
+ return MEMTX_OK;
+ }
+
switch (offset) {
+ case A_CTRL:
+ /* We don't implement the 'data gating' feature so all other bits
+ * are reserved and we make them RAZ/WI.
+ */
+ s->ctrl = value & (R_CTRL_SEC_RESP_MASK |
+ R_CTRL_AUTOINC_MASK |
+ R_CTRL_LOCKDOWN_MASK);
+ break;
+ case A_BLK_IDX:
+ s->blk_idx = value % s->blk_max;
+ break;
+ case A_BLK_LUT:
+ s->blk_lut[s->blk_idx] = value;
+ if (size == 4) {
+ s->blk_idx++;
+ s->blk_idx %= s->blk_max;
+ }
+ break;
+ case A_INT_CLEAR:
+ if (value & R_INT_CLEAR_IRQ_MASK) {
+ s->int_stat = 0;
+ tz_mpc_irq_update(s);
+ }
+ break;
+ case A_INT_EN:
+ s->int_en = value & R_INT_EN_IRQ_MASK;
+ tz_mpc_irq_update(s);
+ break;
+ case A_INT_SET:
+ if (value & R_INT_SET_IRQ_MASK) {
+ s->int_stat = R_INT_STAT_IRQ_MASK;
+ tz_mpc_irq_update(s);
+ }
+ break;
case A_PIDR4:
case A_PIDR5:
case A_PIDR6:
@@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
static void tz_mpc_reset(DeviceState *dev)
{
+ TZMPC *s = TZ_MPC(dev);
+
+ s->ctrl = 0x00000100;
+ s->blk_idx = 0;
+ s->int_stat = 0;
+ s->int_en = 1;
+ s->int_info1 = 0;
+ s->int_info2 = 0;
+
+ memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));
}
static void tz_mpc_init(Object *obj)
@@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)
"tz-mpc-downstream");
address_space_init(&s->blocked_io_as, &s->blocked_io,
"tz-mpc-blocked-io");
+
+ s->blk_lut = g_new(uint32_t, s->blk_max);
+}
+
+static int tz_mpc_post_load(void *opaque, int version_id)
+{
+ TZMPC *s = TZ_MPC(opaque);
+
+ /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */
+ if (s->blk_idx >= s->blk_max) {
+ return -1;
+ }
+ return 0;
}
static const VMStateDescription tz_mpc_vmstate = {
.name = "tz-mpc",
.version_id = 1,
.minimum_version_id = 1,
+ .post_load = tz_mpc_post_load,
.fields = (VMStateField[]) {
+ VMSTATE_UINT32(ctrl, TZMPC),
+ VMSTATE_UINT32(blk_idx, TZMPC),
+ VMSTATE_UINT32(int_stat, TZMPC),
+ VMSTATE_UINT32(int_en, TZMPC),
+ VMSTATE_UINT32(int_info1, TZMPC),
+ VMSTATE_UINT32(int_info2, TZMPC),
+ VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,
+ 0, vmstate_info_uint32, uint32_t),
VMSTATE_END_OF_LIST()
}
};
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers Peter Maydell
@ 2018-06-14 20:14 ` Auger Eric
2018-06-15 8:59 ` Peter Maydell
2018-06-14 20:36 ` Auger Eric
2018-06-15 7:23 ` Auger Eric
2 siblings, 1 reply; 40+ messages in thread
From: Auger Eric @ 2018-06-14 20:14 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini, Peter Xu
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the missing registers for the TZ MPC.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/hw/misc/tz-mpc.h | 10 +++
> hw/misc/tz-mpc.c | 137 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
> index b5eaf1699ea..1fff4d6029a 100644
> --- a/include/hw/misc/tz-mpc.h
> +++ b/include/hw/misc/tz-mpc.h
> @@ -48,6 +48,16 @@ struct TZMPC {
>
> /*< public >*/
>
> + /* State */
> + uint32_t ctrl;
> + uint32_t blk_idx;
> + uint32_t int_stat;
> + uint32_t int_en;
> + uint32_t int_info1;
> + uint32_t int_info2;
> +
> + uint32_t *blk_lut;
> +
> qemu_irq irq;
>
> /* Properties */
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index d4467ccc3b2..9db55e23daf 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -28,16 +28,23 @@ enum {
>
> /* Config registers */
> REG32(CTRL, 0x00)
> + FIELD(CTRL, SEC_RESP, 4, 1)
> + FIELD(CTRL, AUTOINC, 8, 1)
> + FIELD(CTRL, LOCKDOWN, 31, 1)
> REG32(BLK_MAX, 0x10)
> REG32(BLK_CFG, 0x14)
> REG32(BLK_IDX, 0x18)
> REG32(BLK_LUT, 0x1c)
> REG32(INT_STAT, 0x20)
> + FIELD(INT_STAT, IRQ, 0, 1)
> REG32(INT_CLEAR, 0x24)
> + FIELD(INT_CLEAR, IRQ, 0, 1)
> REG32(INT_EN, 0x28)
> + FIELD(INT_EN, IRQ, 0, 1)
> REG32(INT_INFO1, 0x2c)
> REG32(INT_INFO2, 0x30)
> REG32(INT_SET, 0x34)
> + FIELD(INT_SET, IRQ, 0, 1)
> REG32(PIDR4, 0xfd0)
> REG32(PIDR5, 0xfd4)
> REG32(PIDR6, 0xfd8)
> @@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {
> 0x0d, 0xf0, 0x05, 0xb1,
> };
>
> +static void tz_mpc_irq_update(TZMPC *s)
> +{
> + qemu_set_irq(s->irq, s->int_stat && s->int_en);
> +}
> +
> static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
> uint64_t *pdata,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> uint64_t r;
> uint32_t offset = addr & ~0x3;
I don't get where do we check if the access is secure or not. Shouldn't
we test attrs.secure somewhere (PPROT[1] == 0)? Also the spec says
IDregs can be read by any type of access. Where is this differentiation
done?
Thanks
Eric
>
> switch (offset) {
> + case A_CTRL:
> + r = s->ctrl;
> + break;
> + case A_BLK_MAX:
> + r = s->blk_max;
> + break;
> + case A_BLK_CFG:
> + /* We are never in "init in progress state", so this just indicates
> + * the block size. s->blocksize == (1 << BLK_CFG + 5), so
> + * BLK_CFG == ctz32(s->blocksize) - 5
> + */
> + r = ctz32(s->blocksize) - 5;
> + break;
> + case A_BLK_IDX:
> + r = s->blk_idx;
> + break;
> + case A_BLK_LUT:
> + r = s->blk_lut[s->blk_idx];
> + if (size == 4) {
> + s->blk_idx++;
> + s->blk_idx %= s->blk_max;
> + }
> + break;
> + case A_INT_STAT:
> + r = s->int_stat;
> + break;
> + case A_INT_EN:
> + r = s->int_en;
> + break;
> + case A_INT_INFO1:
> + r = s->int_info1;
> + break;
> + case A_INT_INFO2:
> + r = s->int_info2;
> + break;
> case A_PIDR4:
> case A_PIDR5:
> case A_PIDR6:
> @@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> uint64_t value,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> uint32_t offset = addr & ~0x3;
>
> trace_tz_mpc_reg_write(addr, value, size);
> @@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> uint32_t oldval;
>
> switch (offset) {
> - /* As we add support for registers which need expansions
> - * other than zeroes we'll fill in cases here.
> - */
> + case A_CTRL:
> + oldval = s->ctrl;
> + break;
> + case A_BLK_IDX:
> + oldval = s->blk_idx;
> + break;
> + case A_BLK_LUT:
> + oldval = s->blk_lut[s->blk_idx];
> + break;
> default:
> oldval = 0;
> break;
> @@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
> }
>
> + if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&
> + (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {
> + /* Lockdown mode makes these three registers read-only, and
> + * the only way out of it is to reset the device.
> + */
> + qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "
> + "while MPC is in lockdown mode\n", offset);
> + return MEMTX_OK;
> + }
> +
> switch (offset) {
> + case A_CTRL:
> + /* We don't implement the 'data gating' feature so all other bits
> + * are reserved and we make them RAZ/WI.
> + */
> + s->ctrl = value & (R_CTRL_SEC_RESP_MASK |
> + R_CTRL_AUTOINC_MASK |
> + R_CTRL_LOCKDOWN_MASK);
> + break;
> + case A_BLK_IDX:
> + s->blk_idx = value % s->blk_max;
> + break;
> + case A_BLK_LUT:
> + s->blk_lut[s->blk_idx] = value;
> + if (size == 4) {
> + s->blk_idx++;
> + s->blk_idx %= s->blk_max;
> + }
> + break;
> + case A_INT_CLEAR:
> + if (value & R_INT_CLEAR_IRQ_MASK) {
> + s->int_stat = 0;
> + tz_mpc_irq_update(s);
> + }
> + break;
> + case A_INT_EN:
> + s->int_en = value & R_INT_EN_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + break;
> + case A_INT_SET:
> + if (value & R_INT_SET_IRQ_MASK) {
> + s->int_stat = R_INT_STAT_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + }
> + break;
> case A_PIDR4:
> case A_PIDR5:
> case A_PIDR6:
> @@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
>
> static void tz_mpc_reset(DeviceState *dev)
> {
> + TZMPC *s = TZ_MPC(dev);
> +
> + s->ctrl = 0x00000100;
> + s->blk_idx = 0;
> + s->int_stat = 0;
> + s->int_en = 1;
> + s->int_info1 = 0;
> + s->int_info2 = 0;
> +
> + memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));
> }
>
> static void tz_mpc_init(Object *obj)
> @@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)
> "tz-mpc-downstream");
> address_space_init(&s->blocked_io_as, &s->blocked_io,
> "tz-mpc-blocked-io");
> +
> + s->blk_lut = g_new(uint32_t, s->blk_max);
> +}
> +
> +static int tz_mpc_post_load(void *opaque, int version_id)
> +{
> + TZMPC *s = TZ_MPC(opaque);
> +
> + /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */
> + if (s->blk_idx >= s->blk_max) {
> + return -1;
> + }
> + return 0;
> }
>
> static const VMStateDescription tz_mpc_vmstate = {
> .name = "tz-mpc",
> .version_id = 1,
> .minimum_version_id = 1,
> + .post_load = tz_mpc_post_load,
> .fields = (VMStateField[]) {
> + VMSTATE_UINT32(ctrl, TZMPC),
> + VMSTATE_UINT32(blk_idx, TZMPC),
> + VMSTATE_UINT32(int_stat, TZMPC),
> + VMSTATE_UINT32(int_en, TZMPC),
> + VMSTATE_UINT32(int_info1, TZMPC),
> + VMSTATE_UINT32(int_info2, TZMPC),
> + VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,
> + 0, vmstate_info_uint32, uint32_t),
> VMSTATE_END_OF_LIST()
> }
> };
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-14 20:14 ` Auger Eric
@ 2018-06-15 8:59 ` Peter Maydell
0 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 8:59 UTC (permalink / raw)
To: Auger Eric
Cc: qemu-arm, QEMU Developers, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Peter Xu
On 14 June 2018 at 21:14, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,
>
> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>> Implement the missing registers for the TZ MPC.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
>> uint64_t *pdata,
>> unsigned size, MemTxAttrs attrs)
>> {
>> + TZMPC *s = TZ_MPC(opaque);
>> uint64_t r;
>> uint32_t offset = addr & ~0x3;
> I don't get where do we check if the access is secure or not. Shouldn't
> we test attrs.secure somewhere (PPROT[1] == 0)? Also the spec says
> IDregs can be read by any type of access. Where is this differentiation
> done?
I'd missed that bit of the spec -- yes, we should be checking
attrs.secure to see if we should allow access to the non-ID registers.
For the MPS2 boards it doesn't matter because the registers are
in a part of the memory map that is enforced as secure-access-only
by the IDAU.
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers Peter Maydell
2018-06-14 20:14 ` Auger Eric
@ 2018-06-14 20:36 ` Auger Eric
2018-06-15 9:04 ` Peter Maydell
2018-06-15 7:23 ` Auger Eric
2 siblings, 1 reply; 40+ messages in thread
From: Auger Eric @ 2018-06-14 20:36 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini, Peter Xu
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the missing registers for the TZ MPC.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/hw/misc/tz-mpc.h | 10 +++
> hw/misc/tz-mpc.c | 137 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
> index b5eaf1699ea..1fff4d6029a 100644
> --- a/include/hw/misc/tz-mpc.h
> +++ b/include/hw/misc/tz-mpc.h
> @@ -48,6 +48,16 @@ struct TZMPC {
>
> /*< public >*/
>
> + /* State */
> + uint32_t ctrl;
> + uint32_t blk_idx;
> + uint32_t int_stat;
> + uint32_t int_en;
> + uint32_t int_info1;
> + uint32_t int_info2;
> +
> + uint32_t *blk_lut;
> +
> qemu_irq irq;
>
> /* Properties */
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index d4467ccc3b2..9db55e23daf 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -28,16 +28,23 @@ enum {
>
> /* Config registers */
> REG32(CTRL, 0x00)
> + FIELD(CTRL, SEC_RESP, 4, 1)
> + FIELD(CTRL, AUTOINC, 8, 1)
> + FIELD(CTRL, LOCKDOWN, 31, 1)
> REG32(BLK_MAX, 0x10)
> REG32(BLK_CFG, 0x14)
> REG32(BLK_IDX, 0x18)
> REG32(BLK_LUT, 0x1c)
> REG32(INT_STAT, 0x20)
> + FIELD(INT_STAT, IRQ, 0, 1)
> REG32(INT_CLEAR, 0x24)
> + FIELD(INT_CLEAR, IRQ, 0, 1)
> REG32(INT_EN, 0x28)
> + FIELD(INT_EN, IRQ, 0, 1)
> REG32(INT_INFO1, 0x2c)
> REG32(INT_INFO2, 0x30)
> REG32(INT_SET, 0x34)
> + FIELD(INT_SET, IRQ, 0, 1)
> REG32(PIDR4, 0xfd0)
> REG32(PIDR5, 0xfd4)
> REG32(PIDR6, 0xfd8)
> @@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {
> 0x0d, 0xf0, 0x05, 0xb1,
> };
>
> +static void tz_mpc_irq_update(TZMPC *s)
> +{
> + qemu_set_irq(s->irq, s->int_stat && s->int_en);
> +}
> +
> static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
> uint64_t *pdata,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> uint64_t r;
> uint32_t offset = addr & ~0x3;
>
> switch (offset) {
> + case A_CTRL:
> + r = s->ctrl;
> + break;
> + case A_BLK_MAX:
> + r = s->blk_max;
> + break;
> + case A_BLK_CFG:
> + /* We are never in "init in progress state", so this just indicates
> + * the block size. s->blocksize == (1 << BLK_CFG + 5), so
> + * BLK_CFG == ctz32(s->blocksize) - 5
> + */
> + r = ctz32(s->blocksize) - 5;
> + break;
> + case A_BLK_IDX:
> + r = s->blk_idx;
> + break;
> + case A_BLK_LUT:
> + r = s->blk_lut[s->blk_idx];
> + if (size == 4) {
> + s->blk_idx++;
> + s->blk_idx %= s->blk_max;
> + }
> + break;
> + case A_INT_STAT:
> + r = s->int_stat;
> + break;
> + case A_INT_EN:
> + r = s->int_en;
> + break;
> + case A_INT_INFO1:
> + r = s->int_info1;
> + break;
> + case A_INT_INFO2:
> + r = s->int_info2;
> + break;
> case A_PIDR4:
> case A_PIDR5:
> case A_PIDR6:
> @@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> uint64_t value,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> uint32_t offset = addr & ~0x3;
>
> trace_tz_mpc_reg_write(addr, value, size);
> @@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> uint32_t oldval;
>
> switch (offset) {
> - /* As we add support for registers which need expansions
> - * other than zeroes we'll fill in cases here.
> - */
> + case A_CTRL:
> + oldval = s->ctrl;
> + break;
> + case A_BLK_IDX:
> + oldval = s->blk_idx;
> + break;
> + case A_BLK_LUT:
> + oldval = s->blk_lut[s->blk_idx];
> + break;
> default:
> oldval = 0;
> break;
> @@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
> }
>
> + if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&
> + (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {
> + /* Lockdown mode makes these three registers read-only, and
> + * the only way out of it is to reset the device.
> + */
> + qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "
> + "while MPC is in lockdown mode\n", offset);
> + return MEMTX_OK;
> + }
> +
> switch (offset) {
> + case A_CTRL:
> + /* We don't implement the 'data gating' feature so all other bits
> + * are reserved and we make them RAZ/WI.
> + */
> + s->ctrl = value & (R_CTRL_SEC_RESP_MASK |
> + R_CTRL_AUTOINC_MASK |
> + R_CTRL_LOCKDOWN_MASK);
> + break;
> + case A_BLK_IDX:
> + s->blk_idx = value % s->blk_max;
> + break;
> + case A_BLK_LUT:
> + s->blk_lut[s->blk_idx] = value;
> + if (size == 4) {
> + s->blk_idx++;
> + s->blk_idx %= s->blk_max;
> + }
> + break;
> + case A_INT_CLEAR:
> + if (value & R_INT_CLEAR_IRQ_MASK) {
> + s->int_stat = 0;
> + tz_mpc_irq_update(s);
don't you need to clear the info regs. spec says:
the [info] register retains its value until mpc_irq is cleared.
Thanks
Eric
> + }
> + break;
> + case A_INT_EN:
> + s->int_en = value & R_INT_EN_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + break;
> + case A_INT_SET:
> + if (value & R_INT_SET_IRQ_MASK) {
> + s->int_stat = R_INT_STAT_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + }
> + break;
> case A_PIDR4:
> case A_PIDR5:
> case A_PIDR6:
> @@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
>
> static void tz_mpc_reset(DeviceState *dev)
> {
> + TZMPC *s = TZ_MPC(dev);
> +
> + s->ctrl = 0x00000100;
> + s->blk_idx = 0;
> + s->int_stat = 0;
> + s->int_en = 1;
> + s->int_info1 = 0;
> + s->int_info2 = 0;
> +
> + memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));
> }
>
> static void tz_mpc_init(Object *obj)
> @@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)
> "tz-mpc-downstream");
> address_space_init(&s->blocked_io_as, &s->blocked_io,
> "tz-mpc-blocked-io");
> +
> + s->blk_lut = g_new(uint32_t, s->blk_max);
> +}
> +
> +static int tz_mpc_post_load(void *opaque, int version_id)
> +{
> + TZMPC *s = TZ_MPC(opaque);
> +
> + /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */
> + if (s->blk_idx >= s->blk_max) {
> + return -1;
> + }
> + return 0;
> }
>
> static const VMStateDescription tz_mpc_vmstate = {
> .name = "tz-mpc",
> .version_id = 1,
> .minimum_version_id = 1,
> + .post_load = tz_mpc_post_load,
> .fields = (VMStateField[]) {
> + VMSTATE_UINT32(ctrl, TZMPC),
> + VMSTATE_UINT32(blk_idx, TZMPC),
> + VMSTATE_UINT32(int_stat, TZMPC),
> + VMSTATE_UINT32(int_en, TZMPC),
> + VMSTATE_UINT32(int_info1, TZMPC),
> + VMSTATE_UINT32(int_info2, TZMPC),
> + VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,
> + 0, vmstate_info_uint32, uint32_t),
> VMSTATE_END_OF_LIST()
> }
> };
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-14 20:36 ` Auger Eric
@ 2018-06-15 9:04 ` Peter Maydell
2018-06-15 13:24 ` Auger Eric
0 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 9:04 UTC (permalink / raw)
To: Auger Eric
Cc: qemu-arm, QEMU Developers, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Peter Xu
On 14 June 2018 at 21:36, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,
>
> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>> Implement the missing registers for the TZ MPC.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> + case A_INT_CLEAR:
>> + if (value & R_INT_CLEAR_IRQ_MASK) {
>> + s->int_stat = 0;
>> + tz_mpc_irq_update(s);
> don't you need to clear the info regs. spec says:
> the [info] register retains its value until mpc_irq is cleared.
The full sentence is "Subsequent security violating transfers
remain blocked, that is, not captured in this register
and the register retains its value until mpc_irq is cleared."
I interpret "until mpc_irq is cleared" as applying to the
entire thing, ie mpc_irq being cleared is what allows a
subsequent transfer to be captured in this register.
(Hardware actively clearing itself to zero is unlikely,
because that costs extra gates which designers don't tend
to do unless there's a reason for it.)
>From a guest point of view (which is kind of the pov the
docs are written from), the guest can't rely on the register
value once mpc_irq is cleared (because another transaction
might come along and cause the value to be overwritten).
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-15 9:04 ` Peter Maydell
@ 2018-06-15 13:24 ` Auger Eric
0 siblings, 0 replies; 40+ messages in thread
From: Auger Eric @ 2018-06-15 13:24 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, QEMU Developers, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Peter Xu
Hi Peter,
On 06/15/2018 11:04 AM, Peter Maydell wrote:
> On 14 June 2018 at 21:36, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Peter,
>>
>> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>>> Implement the missing registers for the TZ MPC.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>>> + case A_INT_CLEAR:
>>> + if (value & R_INT_CLEAR_IRQ_MASK) {
>>> + s->int_stat = 0;
>>> + tz_mpc_irq_update(s);
>> don't you need to clear the info regs. spec says:
>> the [info] register retains its value until mpc_irq is cleared.
>
> The full sentence is "Subsequent security violating transfers
> remain blocked, that is, not captured in this register
> and the register retains its value until mpc_irq is cleared."
> I interpret "until mpc_irq is cleared" as applying to the
> entire thing, ie mpc_irq being cleared is what allows a
> subsequent transfer to be captured in this register.
> (Hardware actively clearing itself to zero is unlikely,
> because that costs extra gates which designers don't tend
> to do unless there's a reason for it.)
>
> From a guest point of view (which is kind of the pov the
> docs are written from), the guest can't rely on the register
> value once mpc_irq is cleared (because another transaction
> might come along and cause the value to be overwritten).
OK
Thanks
Eric
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers Peter Maydell
2018-06-14 20:14 ` Auger Eric
2018-06-14 20:36 ` Auger Eric
@ 2018-06-15 7:23 ` Auger Eric
2018-06-15 9:05 ` Peter Maydell
2 siblings, 1 reply; 40+ messages in thread
From: Auger Eric @ 2018-06-15 7:23 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini, Peter Xu
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> Implement the missing registers for the TZ MPC.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/hw/misc/tz-mpc.h | 10 +++
> hw/misc/tz-mpc.c | 137 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 144 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/misc/tz-mpc.h b/include/hw/misc/tz-mpc.h
> index b5eaf1699ea..1fff4d6029a 100644
> --- a/include/hw/misc/tz-mpc.h
> +++ b/include/hw/misc/tz-mpc.h
> @@ -48,6 +48,16 @@ struct TZMPC {
>
> /*< public >*/
>
> + /* State */
> + uint32_t ctrl;
> + uint32_t blk_idx;
> + uint32_t int_stat;
> + uint32_t int_en;
> + uint32_t int_info1;
> + uint32_t int_info2;
> +
> + uint32_t *blk_lut;
> +
> qemu_irq irq;
>
> /* Properties */
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index d4467ccc3b2..9db55e23daf 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -28,16 +28,23 @@ enum {
>
> /* Config registers */
> REG32(CTRL, 0x00)
> + FIELD(CTRL, SEC_RESP, 4, 1)
> + FIELD(CTRL, AUTOINC, 8, 1)
> + FIELD(CTRL, LOCKDOWN, 31, 1)
> REG32(BLK_MAX, 0x10)
> REG32(BLK_CFG, 0x14)
> REG32(BLK_IDX, 0x18)
> REG32(BLK_LUT, 0x1c)
> REG32(INT_STAT, 0x20)
> + FIELD(INT_STAT, IRQ, 0, 1)
> REG32(INT_CLEAR, 0x24)
> + FIELD(INT_CLEAR, IRQ, 0, 1)
> REG32(INT_EN, 0x28)
> + FIELD(INT_EN, IRQ, 0, 1)
> REG32(INT_INFO1, 0x2c)
> REG32(INT_INFO2, 0x30)
> REG32(INT_SET, 0x34)
> + FIELD(INT_SET, IRQ, 0, 1)
> REG32(PIDR4, 0xfd0)
> REG32(PIDR5, 0xfd4)
> REG32(PIDR6, 0xfd8)
> @@ -57,14 +64,55 @@ static const uint8_t tz_mpc_idregs[] = {
> 0x0d, 0xf0, 0x05, 0xb1,
> };
>
> +static void tz_mpc_irq_update(TZMPC *s)
> +{
> + qemu_set_irq(s->irq, s->int_stat && s->int_en);
> +}
> +
> static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
> uint64_t *pdata,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> uint64_t r;
> uint32_t offset = addr & ~0x3;
>
> switch (offset) {
> + case A_CTRL:
> + r = s->ctrl;
> + break;
> + case A_BLK_MAX:
> + r = s->blk_max;
> + break;
> + case A_BLK_CFG:
> + /* We are never in "init in progress state", so this just indicates
> + * the block size. s->blocksize == (1 << BLK_CFG + 5), so
> + * BLK_CFG == ctz32(s->blocksize) - 5
> + */
> + r = ctz32(s->blocksize) - 5;
> + break;
> + case A_BLK_IDX:
> + r = s->blk_idx;
> + break;
> + case A_BLK_LUT:
> + r = s->blk_lut[s->blk_idx];
> + if (size == 4) {
don't you need to test CTRL[8] before doing the auto-increment?
spec says:
"A full word write or read to this register
automatically increments the BLK_IDX by one if
enabled by CTRL[8]."
> + s->blk_idx++;
> + s->blk_idx %= s->blk_max;
> + }
> + break;
> + case A_INT_STAT:
> + r = s->int_stat;
> + break;
> + case A_INT_EN:
> + r = s->int_en;
> + break;
> + case A_INT_INFO1:
> + r = s->int_info1;
> + break;
> + case A_INT_INFO2:
> + r = s->int_info2;
> + break;
> case A_PIDR4:
> case A_PIDR5:
> case A_PIDR6:
> @@ -110,6 +158,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> uint64_t value,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> uint32_t offset = addr & ~0x3;
>
> trace_tz_mpc_reg_write(addr, value, size);
> @@ -122,9 +171,15 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> uint32_t oldval;
>
> switch (offset) {
> - /* As we add support for registers which need expansions
> - * other than zeroes we'll fill in cases here.
> - */
> + case A_CTRL:
> + oldval = s->ctrl;
> + break;
> + case A_BLK_IDX:
> + oldval = s->blk_idx;
> + break;
> + case A_BLK_LUT:
> + oldval = s->blk_lut[s->blk_idx];
> + break;
> default:
> oldval = 0;
> break;
> @@ -132,7 +187,51 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> value = deposit32(oldval, (addr & 3) * 8, size * 8, value);
> }
>
> + if ((s->ctrl & R_CTRL_LOCKDOWN_MASK) &&
> + (offset == A_CTRL || offset == A_BLK_LUT || offset == A_INT_EN)) {
> + /* Lockdown mode makes these three registers read-only, and
> + * the only way out of it is to reset the device.
> + */
> + qemu_log_mask(LOG_GUEST_ERROR, "TZ MPC register write to offset 0x%x "
> + "while MPC is in lockdown mode\n", offset);
> + return MEMTX_OK;
> + }
> +
> switch (offset) {
> + case A_CTRL:
> + /* We don't implement the 'data gating' feature so all other bits
> + * are reserved and we make them RAZ/WI.
> + */
> + s->ctrl = value & (R_CTRL_SEC_RESP_MASK |
> + R_CTRL_AUTOINC_MASK |
> + R_CTRL_LOCKDOWN_MASK);
> + break;
> + case A_BLK_IDX:
> + s->blk_idx = value % s->blk_max;
> + break;
> + case A_BLK_LUT:
> + s->blk_lut[s->blk_idx] = value;
> + if (size == 4) {
> + s->blk_idx++;
same?
Thanks
Eric
> + s->blk_idx %= s->blk_max;
> + }
> + break;
> + case A_INT_CLEAR:
> + if (value & R_INT_CLEAR_IRQ_MASK) {
> + s->int_stat = 0;
> + tz_mpc_irq_update(s);
> + }
> + break;
> + case A_INT_EN:
> + s->int_en = value & R_INT_EN_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + break;
> + case A_INT_SET:
> + if (value & R_INT_SET_IRQ_MASK) {
> + s->int_stat = R_INT_STAT_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + }
> + break;
> case A_PIDR4:
> case A_PIDR5:
> case A_PIDR6:
> @@ -248,6 +347,16 @@ static int tz_mpc_num_indexes(IOMMUMemoryRegion *iommu)
>
> static void tz_mpc_reset(DeviceState *dev)
> {
> + TZMPC *s = TZ_MPC(dev);
> +
> + s->ctrl = 0x00000100;
> + s->blk_idx = 0;
> + s->int_stat = 0;
> + s->int_en = 1;
> + s->int_info1 = 0;
> + s->int_info2 = 0;
> +
> + memset(s->blk_lut, 0, s->blk_max * sizeof(uint32_t));
> }
>
> static void tz_mpc_init(Object *obj)
> @@ -321,13 +430,35 @@ static void tz_mpc_realize(DeviceState *dev, Error **errp)
> "tz-mpc-downstream");
> address_space_init(&s->blocked_io_as, &s->blocked_io,
> "tz-mpc-blocked-io");
> +
> + s->blk_lut = g_new(uint32_t, s->blk_max);
> +}
> +
> +static int tz_mpc_post_load(void *opaque, int version_id)
> +{
> + TZMPC *s = TZ_MPC(opaque);
> +
> + /* Check the incoming data doesn't point blk_idx off the end of blk_lut. */
> + if (s->blk_idx >= s->blk_max) {
> + return -1;
> + }
> + return 0;
> }
>
> static const VMStateDescription tz_mpc_vmstate = {
> .name = "tz-mpc",
> .version_id = 1,
> .minimum_version_id = 1,
> + .post_load = tz_mpc_post_load,
> .fields = (VMStateField[]) {
> + VMSTATE_UINT32(ctrl, TZMPC),
> + VMSTATE_UINT32(blk_idx, TZMPC),
> + VMSTATE_UINT32(int_stat, TZMPC),
> + VMSTATE_UINT32(int_en, TZMPC),
> + VMSTATE_UINT32(int_info1, TZMPC),
> + VMSTATE_UINT32(int_info2, TZMPC),
> + VMSTATE_VARRAY_UINT32(blk_lut, TZMPC, blk_max,
> + 0, vmstate_info_uint32, uint32_t),
> VMSTATE_END_OF_LIST()
> }
> };
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers
2018-06-15 7:23 ` Auger Eric
@ 2018-06-15 9:05 ` Peter Maydell
0 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 9:05 UTC (permalink / raw)
To: Auger Eric
Cc: qemu-arm, QEMU Developers, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Peter Xu
On 15 June 2018 at 08:23, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,
>
> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>> Implement the missing registers for the TZ MPC.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> + case A_BLK_LUT:
>> + r = s->blk_lut[s->blk_idx];
>> + if (size == 4) {
> don't you need to test CTRL[8] before doing the auto-increment?
>
> spec says:
> "A full word write or read to this register
> automatically increments the BLK_IDX by one if
> enabled by CTRL[8]."
Oops, yes, definitely.
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 07/13] hw/misc/tz-mpc.c: Implement correct blocked-access behaviour
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (5 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 06/13] hw/misc/tz-mpc.c: Implement registers Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 20:40 ` Auger Eric
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate Peter Maydell
` (8 subsequent siblings)
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
The MPC is guest-configurable for whether blocked accesses:
* should be RAZ/WI or cause a bus error
* should generate an interrupt or not
Implement this behaviour in the blocked-access handlers.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/misc/tz-mpc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index 9db55e23daf..704bb3fb44d 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -43,6 +43,9 @@ REG32(INT_EN, 0x28)
FIELD(INT_EN, IRQ, 0, 1)
REG32(INT_INFO1, 0x2c)
REG32(INT_INFO2, 0x30)
+ FIELD(INT_INFO2, HMASTER, 0, 16)
+ FIELD(INT_INFO2, HNONSEC, 16, 1)
+ FIELD(INT_INFO2, CFG_NS, 17, 1)
REG32(INT_SET, 0x34)
FIELD(INT_SET, IRQ, 0, 1)
REG32(PIDR4, 0xfd0)
@@ -266,6 +269,45 @@ static const MemoryRegionOps tz_mpc_reg_ops = {
.impl.max_access_size = 4,
};
+static inline bool tz_mpc_cfg_ns(TZMPC *s, hwaddr addr)
+{
+ /* Return the cfg_ns bit from the LUT for the specified address */
+ hwaddr blknum = addr / s->blocksize;
+ hwaddr blkword = blknum / 32;
+ uint32_t blkbit = 1U << (blknum % 32);
+
+ /* This would imply the address was larger than the size we
+ * defined this memory region to be, so it can't happen.
+ */
+ assert(blkword < s->blk_max);
+ return s->blk_lut[blkword] & blkbit;
+}
+
+static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs attrs)
+{
+ /* Handle a blocked transaction: raise IRQ, capture info, etc */
+ if (!s->int_stat) {
+ /* First blocked transfer: capture information into INT_INFO1 and
+ * INT_INFO2. Subsequent transfers are still blocked but don't
+ * capture information until the guest clears the interrupt.
+ */
+
+ s->int_info1 = addr;
+ s->int_info2 = 0;
+ s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
+ attrs.requester_id & 0xffff);
+ s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
+ ~attrs.secure);
+ s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
+ tz_mpc_cfg_ns(s, addr));
+ s->int_stat |= R_INT_STAT_IRQ_MASK;
+ tz_mpc_irq_update(s);
+ }
+
+ /* Generate bus error if desired; otherwise RAZ/WI */
+ return (s->ctrl & R_CTRL_SEC_RESP_MASK) ? MEMTX_ERROR : MEMTX_OK;
+}
+
/* Accesses only reach these read and write functions if the MPC is
* blocking them; non-blocked accesses go directly to the downstream
* memory region without passing through this code.
@@ -274,19 +316,23 @@ static MemTxResult tz_mpc_mem_blocked_read(void *opaque, hwaddr addr,
uint64_t *pdata,
unsigned size, MemTxAttrs attrs)
{
+ TZMPC *s = TZ_MPC(opaque);
+
trace_tz_mpc_mem_blocked_read(addr, size, attrs.secure);
*pdata = 0;
- return MEMTX_OK;
+ return tz_mpc_handle_block(s, addr, attrs);
}
static MemTxResult tz_mpc_mem_blocked_write(void *opaque, hwaddr addr,
uint64_t value,
unsigned size, MemTxAttrs attrs)
{
+ TZMPC *s = TZ_MPC(opaque);
+
trace_tz_mpc_mem_blocked_write(addr, value, size, attrs.secure);
- return MEMTX_OK;
+ return tz_mpc_handle_block(s, addr, attrs);
}
static const MemoryRegionOps tz_mpc_mem_blocked_ops = {
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/13] hw/misc/tz-mpc.c: Implement correct blocked-access behaviour
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 07/13] hw/misc/tz-mpc.c: Implement correct blocked-access behaviour Peter Maydell
@ 2018-06-14 20:40 ` Auger Eric
0 siblings, 0 replies; 40+ messages in thread
From: Auger Eric @ 2018-06-14 20:40 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Peter Xu, Paolo Bonzini, Alex Bennée, Richard Henderson
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> The MPC is guest-configurable for whether blocked accesses:
> * should be RAZ/WI or cause a bus error
> * should generate an interrupt or not
>
> Implement this behaviour in the blocked-access handlers.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/misc/tz-mpc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index 9db55e23daf..704bb3fb44d 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -43,6 +43,9 @@ REG32(INT_EN, 0x28)
> FIELD(INT_EN, IRQ, 0, 1)
> REG32(INT_INFO1, 0x2c)
> REG32(INT_INFO2, 0x30)
> + FIELD(INT_INFO2, HMASTER, 0, 16)
> + FIELD(INT_INFO2, HNONSEC, 16, 1)
> + FIELD(INT_INFO2, CFG_NS, 17, 1)
> REG32(INT_SET, 0x34)
> FIELD(INT_SET, IRQ, 0, 1)
> REG32(PIDR4, 0xfd0)
> @@ -266,6 +269,45 @@ static const MemoryRegionOps tz_mpc_reg_ops = {
> .impl.max_access_size = 4,
> };
>
> +static inline bool tz_mpc_cfg_ns(TZMPC *s, hwaddr addr)
> +{
> + /* Return the cfg_ns bit from the LUT for the specified address */
> + hwaddr blknum = addr / s->blocksize;
> + hwaddr blkword = blknum / 32;
> + uint32_t blkbit = 1U << (blknum % 32);
> +
> + /* This would imply the address was larger than the size we
> + * defined this memory region to be, so it can't happen.
> + */
> + assert(blkword < s->blk_max);
> + return s->blk_lut[blkword] & blkbit;
> +}
> +
> +static MemTxResult tz_mpc_handle_block(TZMPC *s, hwaddr addr, MemTxAttrs attrs)
> +{
> + /* Handle a blocked transaction: raise IRQ, capture info, etc */
> + if (!s->int_stat) {
> + /* First blocked transfer: capture information into INT_INFO1 and
> + * INT_INFO2. Subsequent transfers are still blocked but don't
> + * capture information until the guest clears the interrupt.
> + */
> +
> + s->int_info1 = addr;
> + s->int_info2 = 0;
> + s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HMASTER,
> + attrs.requester_id & 0xffff);
> + s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, HNONSEC,
> + ~attrs.secure);
> + s->int_info2 = FIELD_DP32(s->int_info2, INT_INFO2, CFG_NS,
> + tz_mpc_cfg_ns(s, addr));
> + s->int_stat |= R_INT_STAT_IRQ_MASK;
> + tz_mpc_irq_update(s);
> + }
> +
> + /* Generate bus error if desired; otherwise RAZ/WI */
> + return (s->ctrl & R_CTRL_SEC_RESP_MASK) ? MEMTX_ERROR : MEMTX_OK;
> +}
> +
> /* Accesses only reach these read and write functions if the MPC is
> * blocking them; non-blocked accesses go directly to the downstream
> * memory region without passing through this code.
> @@ -274,19 +316,23 @@ static MemTxResult tz_mpc_mem_blocked_read(void *opaque, hwaddr addr,
> uint64_t *pdata,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> +
> trace_tz_mpc_mem_blocked_read(addr, size, attrs.secure);
>
> *pdata = 0;
> - return MEMTX_OK;
> + return tz_mpc_handle_block(s, addr, attrs);
> }
>
> static MemTxResult tz_mpc_mem_blocked_write(void *opaque, hwaddr addr,
> uint64_t value,
> unsigned size, MemTxAttrs attrs)
> {
> + TZMPC *s = TZ_MPC(opaque);
> +
> trace_tz_mpc_mem_blocked_write(addr, value, size, attrs.secure);
>
> - return MEMTX_OK;
> + return tz_mpc_handle_block(s, addr, attrs);
> }
>
> static const MemoryRegionOps tz_mpc_mem_blocked_ops = {
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (6 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 07/13] hw/misc/tz-mpc.c: Implement correct blocked-access behaviour Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-15 7:31 ` Auger Eric
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 09/13] hw/core/or-irq: Support more than 16 inputs to an OR gate Peter Maydell
` (7 subsequent siblings)
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
The final part of the Memory Protection Controller we need to
implement is actually using the BLK_LUT data programmed by the
guest to determine whether to block the transaction or not.
Since this means we now change transaction mappings when
the guest writes to BLK_LUT, we must also call the IOMMU
notifiers at that point.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
hw/misc/tz-mpc.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
hw/misc/trace-events | 1 +
2 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
index 704bb3fb44d..7a6117c90b3 100644
--- a/hw/misc/tz-mpc.c
+++ b/hw/misc/tz-mpc.c
@@ -72,6 +72,50 @@ static void tz_mpc_irq_update(TZMPC *s)
qemu_set_irq(s->irq, s->int_stat && s->int_en);
}
+static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx,
+ uint32_t oldlut, uint32_t newlut)
+{
+ /* Called when the LUT word at lutidx has changed from oldlut to newlut;
+ * must call the IOMMU notifiers for the changed blocks.
+ */
+ IOMMUTLBEntry entry = {
+ .addr_mask = s->blocksize - 1,
+ };
+ hwaddr addr = lutidx * s->blocksize * 32;
+ int i;
+
+ for (i = 0; i < 32; i++, addr += s->blocksize) {
+ if (!((oldlut ^ newlut) & (1 << i))) {
+ continue;
+ }
+ /* This changes the mappings for both the S and the NS space,
+ * so we need to do four notifies: an UNMAP then a MAP for each.
+ */
+
+ trace_tz_mpc_iommu_notify(addr);
+ entry.iova = addr;
+ entry.translated_addr = addr;
+
+ entry.perm = IOMMU_NONE;
+ memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);
+ memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);
+
+ entry.perm = IOMMU_RW;
+ if (newlut & (1 << i)) {
+ entry.target_as = &s->blocked_io_as;
+ } else {
+ entry.target_as = &s->downstream_as;
+ }
+ memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);
+ if (newlut & (1 << i)) {
+ entry.target_as = &s->downstream_as;
+ } else {
+ entry.target_as = &s->blocked_io_as;
+ }
+ memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);
+ }
+}
+
static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
uint64_t *pdata,
unsigned size, MemTxAttrs attrs)
@@ -213,6 +257,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
s->blk_idx = value % s->blk_max;
break;
case A_BLK_LUT:
+ tz_mpc_iommu_notify(s, s->blk_idx, s->blk_lut[s->blk_idx], value);
s->blk_lut[s->blk_idx] = value;
if (size == 4) {
s->blk_idx++;
@@ -362,9 +407,10 @@ static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
/* Look at the per-block configuration for this address, and
* return a TLB entry directing the transaction at either
* downstream_as or blocked_io_as, as appropriate.
- * For the moment, always permit accesses.
+ * If the LUT cfg_ns bit is 1, only non-secure transactions
+ * may pass. If the bit is 0, only secure transactions may pass.
*/
- ok = true;
+ ok = tz_mpc_cfg_ns(s, addr) == (iommu_idx == IOMMU_IDX_NS);
trace_tz_mpc_translate(addr, flags,
iommu_idx == IOMMU_IDX_S ? "S" : "NS",
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 72bf9d57000..c956e1419b7 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -90,6 +90,7 @@ tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs wri
tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"
tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"
tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"
+tz_mpc_iommu_notify(uint64_t addr) "TZ MPC iommu: notifying UNMAP/MAP for 0x%" PRIx64
# hw/misc/tz-ppc.c
tz_ppc_reset(void) "TZ PPC: reset"
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate Peter Maydell
@ 2018-06-15 7:31 ` Auger Eric
2018-06-15 16:07 ` Peter Maydell
0 siblings, 1 reply; 40+ messages in thread
From: Auger Eric @ 2018-06-15 7:31 UTC (permalink / raw)
To: Peter Maydell, qemu-arm, qemu-devel
Cc: patches, Peter Xu, Paolo Bonzini, Alex Bennée, Richard Henderson
Hi Peter,
On 06/04/2018 05:29 PM, Peter Maydell wrote:
> The final part of the Memory Protection Controller we need to
> implement is actually using the BLK_LUT data programmed by the
> guest to determine whether to block the transaction or not.
>
> Since this means we now change transaction mappings when
> the guest writes to BLK_LUT, we must also call the IOMMU
> notifiers at that point.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> hw/misc/tz-mpc.c | 50 ++++++++++++++++++++++++++++++++++++++++++--
> hw/misc/trace-events | 1 +
> 2 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c
> index 704bb3fb44d..7a6117c90b3 100644
> --- a/hw/misc/tz-mpc.c
> +++ b/hw/misc/tz-mpc.c
> @@ -72,6 +72,50 @@ static void tz_mpc_irq_update(TZMPC *s)
> qemu_set_irq(s->irq, s->int_stat && s->int_en);
> }
>
> +static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx,
> + uint32_t oldlut, uint32_t newlut)
> +{
> + /* Called when the LUT word at lutidx has changed from oldlut to newlut;
> + * must call the IOMMU notifiers for the changed blocks.
> + */
> + IOMMUTLBEntry entry = {
> + .addr_mask = s->blocksize - 1,
> + };
> + hwaddr addr = lutidx * s->blocksize * 32;
> + int i;
> +
> + for (i = 0; i < 32; i++, addr += s->blocksize) {
> + if (!((oldlut ^ newlut) & (1 << i))) {
> + continue;
> + }
> + /* This changes the mappings for both the S and the NS space,
> + * so we need to do four notifies: an UNMAP then a MAP for each.
> + */
> +
> + trace_tz_mpc_iommu_notify(addr);
> + entry.iova = addr;
> + entry.translated_addr = addr;
> +
> + entry.perm = IOMMU_NONE;
> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);
> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);
> +
> + entry.perm = IOMMU_RW;
> + if (newlut & (1 << i)) {
Add a comment saying the block is configured as non-secure?
> + entry.target_as = &s->blocked_io_as;
> + } else {
> + entry.target_as = &s->downstream_as;
so this disallows any secure access to non secure blocks. I have a
reduced knowledge of TZ but I would have thought that a secure access
would have sufficient priviledge to access NS memory?
Thanks
Eric
> + }
> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry);
> + if (newlut & (1 << i)) {
> + entry.target_as = &s->downstream_as;
> + } else {
> + entry.target_as = &s->blocked_io_as;
> + }
> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry);
> + }
> +}
> +
> static MemTxResult tz_mpc_reg_read(void *opaque, hwaddr addr,
> uint64_t *pdata,
> unsigned size, MemTxAttrs attrs)
> @@ -213,6 +257,7 @@ static MemTxResult tz_mpc_reg_write(void *opaque, hwaddr addr,
> s->blk_idx = value % s->blk_max;
> break;
> case A_BLK_LUT:
> + tz_mpc_iommu_notify(s, s->blk_idx, s->blk_lut[s->blk_idx], value);
> s->blk_lut[s->blk_idx] = value;
> if (size == 4) {
> s->blk_idx++;
> @@ -362,9 +407,10 @@ static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
> /* Look at the per-block configuration for this address, and
> * return a TLB entry directing the transaction at either
> * downstream_as or blocked_io_as, as appropriate.
> - * For the moment, always permit accesses.
> + * If the LUT cfg_ns bit is 1, only non-secure transactions
> + * may pass. If the bit is 0, only secure transactions may pass.
> */
> - ok = true;
> + ok = tz_mpc_cfg_ns(s, addr) == (iommu_idx == IOMMU_IDX_NS);
>
> trace_tz_mpc_translate(addr, flags,
> iommu_idx == IOMMU_IDX_S ? "S" : "NS",
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 72bf9d57000..c956e1419b7 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -90,6 +90,7 @@ tz_mpc_reg_write(uint32_t offset, uint64_t data, unsigned size) "TZ MPC regs wri
> tz_mpc_mem_blocked_read(uint64_t addr, unsigned size, bool secure) "TZ MPC blocked read: offset 0x%" PRIx64 " size %u secure %d"
> tz_mpc_mem_blocked_write(uint64_t addr, uint64_t data, unsigned size, bool secure) "TZ MPC blocked write: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u secure %d"
> tz_mpc_translate(uint64_t addr, int flags, const char *idx, const char *res) "TZ MPC translate: addr 0x%" PRIx64 " flags 0x%x iommu_idx %s: %s"
> +tz_mpc_iommu_notify(uint64_t addr) "TZ MPC iommu: notifying UNMAP/MAP for 0x%" PRIx64
>
> # hw/misc/tz-ppc.c
> tz_ppc_reset(void) "TZ PPC: reset"
>
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
2018-06-15 7:31 ` Auger Eric
@ 2018-06-15 16:07 ` Peter Maydell
2018-06-15 16:09 ` Peter Maydell
0 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 16:07 UTC (permalink / raw)
To: Auger Eric
Cc: qemu-arm, QEMU Developers, patches, Peter Xu, Paolo Bonzini,
Alex Bennée, Richard Henderson
On 15 June 2018 at 08:31, Auger Eric <eric.auger@redhat.com> wrote:
> Hi Peter,
>
> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>> The final part of the Memory Protection Controller we need to
>> implement is actually using the BLK_LUT data programmed by the
>> guest to determine whether to block the transaction or not.
>>
>> Since this means we now change transaction mappings when
>> the guest writes to BLK_LUT, we must also call the IOMMU
>> notifiers at that point.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> + if (newlut & (1 << i)) {
> Add a comment saying the block is configured as non-secure?
Sure.
>> + entry.target_as = &s->blocked_io_as;
>> + } else {
>> + entry.target_as = &s->downstream_as;
> so this disallows any secure access to non secure blocks. I have a
> reduced knowledge of TZ but I would have thought that a secure access
> would have sufficient priviledge to access NS memory?
The spec is not as clear as it could be on this point, but...
>> @@ -362,9 +407,10 @@ static IOMMUTLBEntry tz_mpc_translate(IOMMUMemoryRegion *iommu,
>> /* Look at the per-block configuration for this address, and
>> * return a TLB entry directing the transaction at either
>> * downstream_as or blocked_io_as, as appropriate.
>> - * For the moment, always permit accesses.
>> + * If the LUT cfg_ns bit is 1, only non-secure transactions
>> + * may pass. If the bit is 0, only secure transactions may pass.
>> */
...as this comment says, the MPC doesn't permit wrong-secure-state
transactions either way. (This is the same behaviour as for the
Peripheral Protection Controller, whose spec is clearer.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
2018-06-15 16:07 ` Peter Maydell
@ 2018-06-15 16:09 ` Peter Maydell
2018-06-18 7:45 ` Auger Eric
0 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 16:09 UTC (permalink / raw)
To: Auger Eric
Cc: qemu-arm, QEMU Developers, patches, Peter Xu, Paolo Bonzini,
Alex Bennée, Richard Henderson
On 15 June 2018 at 17:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 June 2018 at 08:31, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Peter,
>>
>> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>>> The final part of the Memory Protection Controller we need to
>>> implement is actually using the BLK_LUT data programmed by the
>>> guest to determine whether to block the transaction or not.
>>>
>>> Since this means we now change transaction mappings when
>>> the guest writes to BLK_LUT, we must also call the IOMMU
>>> notifiers at that point.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>>> + if (newlut & (1 << i)) {
>> Add a comment saying the block is configured as non-secure?
...actually, how about instead having a new variable
bool block_is_ns = newlut & (1 << i);
and then here and in the later if() having "if (block_is_ns)" ?
I think that clarifies without needing a comment.
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
2018-06-15 16:09 ` Peter Maydell
@ 2018-06-18 7:45 ` Auger Eric
0 siblings, 0 replies; 40+ messages in thread
From: Auger Eric @ 2018-06-18 7:45 UTC (permalink / raw)
To: Peter Maydell
Cc: patches, QEMU Developers, Peter Xu, qemu-arm, Paolo Bonzini,
Alex Bennée, Richard Henderson
Hi Peter,
On 06/15/2018 06:09 PM, Peter Maydell wrote:
> On 15 June 2018 at 17:07, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 15 June 2018 at 08:31, Auger Eric <eric.auger@redhat.com> wrote:
>>> Hi Peter,
>>>
>>> On 06/04/2018 05:29 PM, Peter Maydell wrote:
>>>> The final part of the Memory Protection Controller we need to
>>>> implement is actually using the BLK_LUT data programmed by the
>>>> guest to determine whether to block the transaction or not.
>>>>
>>>> Since this means we now change transaction mappings when
>>>> the guest writes to BLK_LUT, we must also call the IOMMU
>>>> notifiers at that point.
>>>>
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>>>> + if (newlut & (1 << i)) {
>>> Add a comment saying the block is configured as non-secure?
>
> ...actually, how about instead having a new variable
> bool block_is_ns = newlut & (1 << i);
>
> and then here and in the later if() having "if (block_is_ns)" ?
> I think that clarifies without needing a comment.
yes, looks good to me
Thanks
Eric
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 09/13] hw/core/or-irq: Support more than 16 inputs to an OR gate
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (7 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 08/13] hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-14 18:24 ` Alex Bennée
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 10/13] hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS Peter Maydell
` (6 subsequent siblings)
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
For the IoTKit MPC support, we need to wire together the
interrupt outputs of 17 MPCs; this exceeds the current
value of MAX_OR_LINES. Increase MAX_OR_LINES to 32 (which
should be enough for anyone).
The tricky part is retaining the migration compatibility for
existing OR gates; we add a subsection which is only used
for larger OR gates, and define it such that we can freely
increase MAX_OR_LINES in future (or even move to a dynamically
allocated levels[] array without an upper size limit) without
breaking compatibility.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
include/hw/or-irq.h | 5 ++++-
hw/core/or-irq.c | 39 +++++++++++++++++++++++++++++++++++++--
2 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/include/hw/or-irq.h b/include/hw/or-irq.h
index 3f6fc1b58a4..5a31e5a1881 100644
--- a/include/hw/or-irq.h
+++ b/include/hw/or-irq.h
@@ -31,7 +31,10 @@
#define TYPE_OR_IRQ "or-irq"
-#define MAX_OR_LINES 16
+/* This can safely be increased if necessary without breaking
+ * migration compatibility (as long as it remains greater than 15).
+ */
+#define MAX_OR_LINES 32
typedef struct OrIRQState qemu_or_irq;
diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
index f9d76c46415..a86901b673c 100644
--- a/hw/core/or-irq.c
+++ b/hw/core/or-irq.c
@@ -66,14 +66,49 @@ static void or_irq_init(Object *obj)
qdev_init_gpio_out(DEVICE(obj), &s->out_irq, 1);
}
+/* The original version of this device had a fixed 16 entries in its
+ * VMState array; devices with more inputs than this need to
+ * migrate the extra lines via a subsection.
+ * The subsection migrates as much of the levels[] array as is needed
+ * (including repeating the first 16 elements), to avoid the awkwardness
+ * of splitting it in two to meet the requirements of VMSTATE_VARRAY_UINT16.
+ */
+#define OLD_MAX_OR_LINES 16
+#if MAX_OR_LINES < OLD_MAX_OR_LINES
+#error MAX_OR_LINES must be at least 16 for migration compatibility
+#endif
+
+static bool vmstate_extras_needed(void *opaque)
+{
+ qemu_or_irq *s = OR_IRQ(opaque);
+
+ return s->num_lines >= OLD_MAX_OR_LINES;
+}
+
+static const VMStateDescription vmstate_or_irq_extras = {
+ .name = "or-irq-extras",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmstate_extras_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_VARRAY_UINT16_UNSAFE(levels, qemu_or_irq, num_lines, 0,
+ vmstate_info_bool, bool),
+ VMSTATE_END_OF_LIST(),
+ },
+};
+
static const VMStateDescription vmstate_or_irq = {
.name = TYPE_OR_IRQ,
.version_id = 1,
.minimum_version_id = 1,
.fields = (VMStateField[]) {
- VMSTATE_BOOL_ARRAY(levels, qemu_or_irq, MAX_OR_LINES),
+ VMSTATE_BOOL_SUB_ARRAY(levels, qemu_or_irq, 0, OLD_MAX_OR_LINES),
VMSTATE_END_OF_LIST(),
- }
+ },
+ .subsections = (const VMStateDescription*[]) {
+ &vmstate_or_irq_extras,
+ NULL
+ },
};
static Property or_irq_properties[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/13] hw/core/or-irq: Support more than 16 inputs to an OR gate
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 09/13] hw/core/or-irq: Support more than 16 inputs to an OR gate Peter Maydell
@ 2018-06-14 18:24 ` Alex Bennée
0 siblings, 0 replies; 40+ messages in thread
From: Alex Bennée @ 2018-06-14 18:24 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, patches, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Peter Maydell <peter.maydell@linaro.org> writes:
> For the IoTKit MPC support, we need to wire together the
> interrupt outputs of 17 MPCs; this exceeds the current
> value of MAX_OR_LINES. Increase MAX_OR_LINES to 32 (which
> should be enough for anyone).
>
> The tricky part is retaining the migration compatibility for
> existing OR gates; we add a subsection which is only used
> for larger OR gates, and define it such that we can freely
> increase MAX_OR_LINES in future (or even move to a dynamically
> allocated levels[] array without an upper size limit) without
> breaking compatibility.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/hw/or-irq.h | 5 ++++-
> hw/core/or-irq.c | 39 +++++++++++++++++++++++++++++++++++++--
> 2 files changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/or-irq.h b/include/hw/or-irq.h
> index 3f6fc1b58a4..5a31e5a1881 100644
> --- a/include/hw/or-irq.h
> +++ b/include/hw/or-irq.h
> @@ -31,7 +31,10 @@
>
> #define TYPE_OR_IRQ "or-irq"
>
> -#define MAX_OR_LINES 16
> +/* This can safely be increased if necessary without breaking
> + * migration compatibility (as long as it remains greater than 15).
> + */
> +#define MAX_OR_LINES 32
>
> typedef struct OrIRQState qemu_or_irq;
>
> diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
> index f9d76c46415..a86901b673c 100644
> --- a/hw/core/or-irq.c
> +++ b/hw/core/or-irq.c
> @@ -66,14 +66,49 @@ static void or_irq_init(Object *obj)
> qdev_init_gpio_out(DEVICE(obj), &s->out_irq, 1);
> }
>
> +/* The original version of this device had a fixed 16 entries in its
> + * VMState array; devices with more inputs than this need to
> + * migrate the extra lines via a subsection.
> + * The subsection migrates as much of the levels[] array as is needed
> + * (including repeating the first 16 elements), to avoid the awkwardness
> + * of splitting it in two to meet the requirements of VMSTATE_VARRAY_UINT16.
> + */
> +#define OLD_MAX_OR_LINES 16
> +#if MAX_OR_LINES < OLD_MAX_OR_LINES
> +#error MAX_OR_LINES must be at least 16 for migration compatibility
> +#endif
> +
> +static bool vmstate_extras_needed(void *opaque)
> +{
> + qemu_or_irq *s = OR_IRQ(opaque);
> +
> + return s->num_lines >= OLD_MAX_OR_LINES;
> +}
> +
> +static const VMStateDescription vmstate_or_irq_extras = {
> + .name = "or-irq-extras",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = vmstate_extras_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_VARRAY_UINT16_UNSAFE(levels, qemu_or_irq, num_lines, 0,
> + vmstate_info_bool, bool),
> + VMSTATE_END_OF_LIST(),
> + },
> +};
> +
> static const VMStateDescription vmstate_or_irq = {
> .name = TYPE_OR_IRQ,
> .version_id = 1,
> .minimum_version_id = 1,
> .fields = (VMStateField[]) {
> - VMSTATE_BOOL_ARRAY(levels, qemu_or_irq, MAX_OR_LINES),
> + VMSTATE_BOOL_SUB_ARRAY(levels, qemu_or_irq, 0, OLD_MAX_OR_LINES),
> VMSTATE_END_OF_LIST(),
> - }
> + },
> + .subsections = (const VMStateDescription*[]) {
> + &vmstate_or_irq_extras,
> + NULL
> + },
> };
>
> static Property or_irq_properties[] = {
--
Alex Bennée
^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v2 10/13] hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (8 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 09/13] hw/core/or-irq: Support more than 16 inputs to an OR gate Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 11/13] hw/arm/iotkit: Instantiate MPC Peter Maydell
` (5 subsequent siblings)
15 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Implement the SECMPCINTSTATUS register. This is the only register
in the security controller that deals with Memory Protection
Controllers, and it simply provides a read-only view of the
interrupt lines from the various MPCs in the system.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
include/hw/misc/iotkit-secctl.h | 8 +++++++
hw/misc/iotkit-secctl.c | 38 +++++++++++++++++++++++++++++++--
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/include/hw/misc/iotkit-secctl.h b/include/hw/misc/iotkit-secctl.h
index faad0c91901..082c14c925e 100644
--- a/include/hw/misc/iotkit-secctl.h
+++ b/include/hw/misc/iotkit-secctl.h
@@ -39,6 +39,11 @@
* + named GPIO outputs ahb_ppcexp{0,1,2,3}_irq_enable
* + named GPIO outputs ahb_ppcexp{0,1,2,3}_irq_clear
* + named GPIO inputs ahb_ppcexp{0,1,2,3}_irq_status
+ * Controlling the MPC in the IoTKit:
+ * + named GPIO input mpc_status
+ * Controlling each of the 16 expansion MPCs which a system using the IoTKit
+ * might provide:
+ * + named GPIO inputs mpcexp_status[0..15]
*/
#ifndef IOTKIT_SECCTL_H
@@ -55,6 +60,8 @@
#define IOTS_NUM_APB_PPC 2
#define IOTS_NUM_APB_EXP_PPC 4
#define IOTS_NUM_AHB_EXP_PPC 4
+#define IOTS_NUM_EXP_MPC 16
+#define IOTS_NUM_MPC 1
typedef struct IoTKitSecCtl IoTKitSecCtl;
@@ -94,6 +101,7 @@ struct IoTKitSecCtl {
uint32_t secrespcfg;
uint32_t nsccfg;
uint32_t brginten;
+ uint32_t mpcintstatus;
IoTKitSecCtlPPC apb[IOTS_NUM_APB_PPC];
IoTKitSecCtlPPC apbexp[IOTS_NUM_APB_EXP_PPC];
diff --git a/hw/misc/iotkit-secctl.c b/hw/misc/iotkit-secctl.c
index ddd1584d341..de4fd8e36d2 100644
--- a/hw/misc/iotkit-secctl.c
+++ b/hw/misc/iotkit-secctl.c
@@ -139,6 +139,9 @@ static MemTxResult iotkit_secctl_s_read(void *opaque, hwaddr addr,
case A_NSCCFG:
r = s->nsccfg;
break;
+ case A_SECMPCINTSTATUS:
+ r = s->mpcintstatus;
+ break;
case A_SECPPCINTSTAT:
r = s->secppcintstat;
break;
@@ -186,7 +189,6 @@ static MemTxResult iotkit_secctl_s_read(void *opaque, hwaddr addr,
case A_APBSPPPCEXP3:
r = s->apbexp[offset_to_ppc_idx(offset)].sp;
break;
- case A_SECMPCINTSTATUS:
case A_SECMSCINTSTAT:
case A_SECMSCINTEN:
case A_NSMSCEXP:
@@ -572,6 +574,20 @@ static void iotkit_secctl_reset(DeviceState *dev)
foreach_ppc(s, iotkit_secctl_reset_ppc);
}
+static void iotkit_secctl_mpc_status(void *opaque, int n, int level)
+{
+ IoTKitSecCtl *s = IOTKIT_SECCTL(opaque);
+
+ s->mpcintstatus = deposit32(s->mpcintstatus, 0, 1, !!level);
+}
+
+static void iotkit_secctl_mpcexp_status(void *opaque, int n, int level)
+{
+ IoTKitSecCtl *s = IOTKIT_SECCTL(opaque);
+
+ s->mpcintstatus = deposit32(s->mpcintstatus, n + 16, 1, !!level);
+}
+
static void iotkit_secctl_ppc_irqstatus(void *opaque, int n, int level)
{
IoTKitSecCtlPPC *ppc = opaque;
@@ -640,6 +656,10 @@ static void iotkit_secctl_init(Object *obj)
qdev_init_gpio_out_named(dev, &s->sec_resp_cfg, "sec_resp_cfg", 1);
qdev_init_gpio_out_named(dev, &s->nsc_cfg_irq, "nsc_cfg", 1);
+ qdev_init_gpio_in_named(dev, iotkit_secctl_mpc_status, "mpc_status", 1);
+ qdev_init_gpio_in_named(dev, iotkit_secctl_mpcexp_status,
+ "mpcexp_status", IOTS_NUM_EXP_MPC);
+
memory_region_init_io(&s->s_regs, obj, &iotkit_secctl_s_ops,
s, "iotkit-secctl-s-regs", 0x1000);
memory_region_init_io(&s->ns_regs, obj, &iotkit_secctl_ns_ops,
@@ -660,6 +680,16 @@ static const VMStateDescription iotkit_secctl_ppc_vmstate = {
}
};
+static const VMStateDescription iotkit_secctl_mpcintstatus_vmstate = {
+ .name = "iotkit-secctl-mpcintstatus",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(mpcintstatus, IoTKitSecCtl),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
static const VMStateDescription iotkit_secctl_vmstate = {
.name = "iotkit-secctl",
.version_id = 1,
@@ -677,7 +707,11 @@ static const VMStateDescription iotkit_secctl_vmstate = {
VMSTATE_STRUCT_ARRAY(ahbexp, IoTKitSecCtl, IOTS_NUM_AHB_EXP_PPC, 1,
iotkit_secctl_ppc_vmstate, IoTKitSecCtlPPC),
VMSTATE_END_OF_LIST()
- }
+ },
+ .subsections = (const VMStateDescription*[]) {
+ &iotkit_secctl_mpcintstatus_vmstate,
+ NULL
+ },
};
static void iotkit_secctl_class_init(ObjectClass *klass, void *data)
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [Qemu-devel] [PATCH v2 11/13] hw/arm/iotkit: Instantiate MPC
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (9 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 10/13] hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 12/13] hw/arm/iotkit: Wire up MPC interrupt lines Peter Maydell
` (4 subsequent siblings)
15 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Wire up the one MPC that is part of the IoTKit itself. For the
moment we don't wire up its interrupt line.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
include/hw/arm/iotkit.h | 2 ++
hw/arm/iotkit.c | 38 +++++++++++++++++++++++++++-----------
2 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/include/hw/arm/iotkit.h b/include/hw/arm/iotkit.h
index c6129d926b6..b21cf1ab9d1 100644
--- a/include/hw/arm/iotkit.h
+++ b/include/hw/arm/iotkit.h
@@ -51,6 +51,7 @@
#include "hw/arm/armv7m.h"
#include "hw/misc/iotkit-secctl.h"
#include "hw/misc/tz-ppc.h"
+#include "hw/misc/tz-mpc.h"
#include "hw/timer/cmsdk-apb-timer.h"
#include "hw/misc/unimp.h"
#include "hw/or-irq.h"
@@ -74,6 +75,7 @@ typedef struct IoTKit {
IoTKitSecCtl secctl;
TZPPC apb_ppc0;
TZPPC apb_ppc1;
+ TZMPC mpc;
CMSDKAPBTIMER timer0;
CMSDKAPBTIMER timer1;
qemu_or_irq ppc_irq_orgate;
diff --git a/hw/arm/iotkit.c b/hw/arm/iotkit.c
index 234185e8f78..160e40c7449 100644
--- a/hw/arm/iotkit.c
+++ b/hw/arm/iotkit.c
@@ -130,6 +130,7 @@ static void iotkit_init(Object *obj)
TYPE_TZ_PPC);
init_sysbus_child(obj, "apb-ppc1", &s->apb_ppc1, sizeof(s->apb_ppc1),
TYPE_TZ_PPC);
+ init_sysbus_child(obj, "mpc", &s->mpc, sizeof(s->mpc), TYPE_TZ_MPC);
init_sysbus_child(obj, "timer0", &s->timer0, sizeof(s->timer0),
TYPE_CMSDK_APB_TIMER);
init_sysbus_child(obj, "timer1", &s->timer1, sizeof(s->timer1),
@@ -266,15 +267,6 @@ static void iotkit_realize(DeviceState *dev, Error **errp)
*/
make_alias(s, &s->alias3, "alias 3", 0x50000000, 0x10000000, 0x40000000);
- /* This RAM should be behind a Memory Protection Controller, but we
- * don't implement that yet.
- */
- memory_region_init_ram(&s->sram0, NULL, "iotkit.sram0", 0x00008000, &err);
- if (err) {
- error_propagate(errp, err);
- return;
- }
- memory_region_add_subregion(&s->container, 0x20000000, &s->sram0);
/* Security controller */
object_property_set_bool(OBJECT(&s->secctl), true, "realized", &err);
@@ -310,6 +302,32 @@ static void iotkit_realize(DeviceState *dev, Error **errp)
qdev_connect_gpio_out_named(dev_secctl, "sec_resp_cfg", 0,
qdev_get_gpio_in(dev_splitter, 0));
+ /* This RAM lives behind the Memory Protection Controller */
+ memory_region_init_ram(&s->sram0, NULL, "iotkit.sram0", 0x00008000, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ object_property_set_link(OBJECT(&s->mpc), OBJECT(&s->sram0),
+ "downstream", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ object_property_set_bool(OBJECT(&s->mpc), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ /* Map the upstream end of the MPC into the right place... */
+ memory_region_add_subregion(&s->container, 0x20000000,
+ sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->mpc),
+ 1));
+ /* ...and its register interface */
+ memory_region_add_subregion(&s->container, 0x50083000,
+ sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->mpc),
+ 0));
+
/* Devices behind APB PPC0:
* 0x40000000: timer0
* 0x40001000: timer1
@@ -473,8 +491,6 @@ static void iotkit_realize(DeviceState *dev, Error **errp)
create_unimplemented_device("NS watchdog", 0x40081000, 0x1000);
create_unimplemented_device("S watchdog", 0x50081000, 0x1000);
- create_unimplemented_device("SRAM0 MPC", 0x50083000, 0x1000);
-
for (i = 0; i < ARRAY_SIZE(s->ppc_irq_splitter); i++) {
Object *splitter = OBJECT(&s->ppc_irq_splitter[i]);
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [Qemu-devel] [PATCH v2 12/13] hw/arm/iotkit: Wire up MPC interrupt lines
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (10 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 11/13] hw/arm/iotkit: Instantiate MPC Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 13/13] hw/arm/mps2-tz.c: Instantiate MPCs Peter Maydell
` (3 subsequent siblings)
15 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
The interrupt outputs from the MPC in the IoTKit and the expansion
MPCs in the board must be wired up to the security controller, and
also all ORed together to produce a single line to the NVIC.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
include/hw/arm/iotkit.h | 6 ++++
hw/arm/iotkit.c | 74 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+)
diff --git a/include/hw/arm/iotkit.h b/include/hw/arm/iotkit.h
index b21cf1ab9d1..2cddde55dd1 100644
--- a/include/hw/arm/iotkit.h
+++ b/include/hw/arm/iotkit.h
@@ -42,6 +42,9 @@
* + named GPIO outputs ahb_ppcexp{0,1,2,3}_irq_enable
* + named GPIO outputs ahb_ppcexp{0,1,2,3}_irq_clear
* + named GPIO inputs ahb_ppcexp{0,1,2,3}_irq_status
+ * Controlling each of the 16 expansion MPCs which a system using the IoTKit
+ * might provide:
+ * + named GPIO inputs mpcexp_status[0..15]
*/
#ifndef IOTKIT_H
@@ -81,6 +84,8 @@ typedef struct IoTKit {
qemu_or_irq ppc_irq_orgate;
SplitIRQ sec_resp_splitter;
SplitIRQ ppc_irq_splitter[NUM_PPCS];
+ SplitIRQ mpc_irq_splitter[IOTS_NUM_EXP_MPC + IOTS_NUM_MPC];
+ qemu_or_irq mpc_irq_orgate;
UnimplementedDeviceState dualtimer;
UnimplementedDeviceState s32ktimer;
@@ -99,6 +104,7 @@ typedef struct IoTKit {
qemu_irq nsc_cfg_in;
qemu_irq irq_status_in[NUM_EXTERNAL_PPCS];
+ qemu_irq mpcexp_status_in[IOTS_NUM_EXP_MPC];
uint32_t nsccfg;
diff --git a/hw/arm/iotkit.c b/hw/arm/iotkit.c
index 160e40c7449..133d5bb34f4 100644
--- a/hw/arm/iotkit.c
+++ b/hw/arm/iotkit.c
@@ -131,6 +131,18 @@ static void iotkit_init(Object *obj)
init_sysbus_child(obj, "apb-ppc1", &s->apb_ppc1, sizeof(s->apb_ppc1),
TYPE_TZ_PPC);
init_sysbus_child(obj, "mpc", &s->mpc, sizeof(s->mpc), TYPE_TZ_MPC);
+ object_initialize(&s->mpc_irq_orgate, sizeof(s->mpc_irq_orgate),
+ TYPE_OR_IRQ);
+ object_property_add_child(obj, "mpc-irq-orgate",
+ OBJECT(&s->mpc_irq_orgate), &error_abort);
+ for (i = 0; i < ARRAY_SIZE(s->mpc_irq_splitter); i++) {
+ char *name = g_strdup_printf("mpc-irq-splitter-%d", i);
+ SplitIRQ *splitter = &s->mpc_irq_splitter[i];
+
+ object_initialize(splitter, sizeof(*splitter), TYPE_SPLIT_IRQ);
+ object_property_add_child(obj, name, OBJECT(splitter), &error_abort);
+ g_free(name);
+ }
init_sysbus_child(obj, "timer0", &s->timer0, sizeof(s->timer0),
TYPE_CMSDK_APB_TIMER);
init_sysbus_child(obj, "timer1", &s->timer1, sizeof(s->timer1),
@@ -163,6 +175,12 @@ static void iotkit_exp_irq(void *opaque, int n, int level)
qemu_set_irq(s->exp_irqs[n], level);
}
+static void iotkit_mpcexp_status(void *opaque, int n, int level)
+{
+ IoTKit *s = IOTKIT(opaque);
+ qemu_set_irq(s->mpcexp_status_in[n], level);
+}
+
static void iotkit_realize(DeviceState *dev, Error **errp)
{
IoTKit *s = IOTKIT(dev);
@@ -328,6 +346,22 @@ static void iotkit_realize(DeviceState *dev, Error **errp)
sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->mpc),
0));
+ /* We must OR together lines from the MPC splitters to go to the NVIC */
+ object_property_set_int(OBJECT(&s->mpc_irq_orgate),
+ IOTS_NUM_EXP_MPC + IOTS_NUM_MPC, "num-lines", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ object_property_set_bool(OBJECT(&s->mpc_irq_orgate), true,
+ "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ qdev_connect_gpio_out(DEVICE(&s->mpc_irq_orgate), 0,
+ qdev_get_gpio_in(DEVICE(&s->armv7m), 9));
+
/* Devices behind APB PPC0:
* 0x40000000: timer0
* 0x40001000: timer1
@@ -536,6 +570,46 @@ static void iotkit_realize(DeviceState *dev, Error **errp)
g_free(gpioname);
}
+ /* Wire up the splitters for the MPC IRQs */
+ for (i = 0; i < IOTS_NUM_EXP_MPC + IOTS_NUM_MPC; i++) {
+ SplitIRQ *splitter = &s->mpc_irq_splitter[i];
+ DeviceState *dev_splitter = DEVICE(splitter);
+
+ object_property_set_int(OBJECT(splitter), 2, "num-lines", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+ object_property_set_bool(OBJECT(splitter), true, "realized", &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
+
+ if (i < IOTS_NUM_EXP_MPC) {
+ /* Splitter input is from GPIO input line */
+ s->mpcexp_status_in[i] = qdev_get_gpio_in(dev_splitter, 0);
+ qdev_connect_gpio_out(dev_splitter, 0,
+ qdev_get_gpio_in_named(dev_secctl,
+ "mpcexp_status", i));
+ } else {
+ /* Splitter input is from our own MPC */
+ qdev_connect_gpio_out_named(DEVICE(&s->mpc), "irq", 0,
+ qdev_get_gpio_in(dev_splitter, 0));
+ qdev_connect_gpio_out(dev_splitter, 0,
+ qdev_get_gpio_in_named(dev_secctl,
+ "mpc_status", 0));
+ }
+
+ qdev_connect_gpio_out(dev_splitter, 1,
+ qdev_get_gpio_in(DEVICE(&s->mpc_irq_orgate), i));
+ }
+ /* Create GPIO inputs which will pass the line state for our
+ * mpcexp_irq inputs to the correct splitter devices.
+ */
+ qdev_init_gpio_in_named(dev, iotkit_mpcexp_status, "mpcexp_status",
+ IOTS_NUM_EXP_MPC);
+
iotkit_forward_sec_resp_cfg(s);
system_clock_scale = NANOSECONDS_PER_SECOND / s->mainclk_frq;
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* [Qemu-devel] [PATCH v2 13/13] hw/arm/mps2-tz.c: Instantiate MPCs
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (11 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 12/13] hw/arm/iotkit: Wire up MPC interrupt lines Peter Maydell
@ 2018-06-04 15:29 ` Peter Maydell
2018-06-04 16:33 ` [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC no-reply
` (2 subsequent siblings)
15 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-04 15:29 UTC (permalink / raw)
To: qemu-arm, qemu-devel
Cc: patches, Alex Bennée, Richard Henderson, Paolo Bonzini,
Peter Xu, Eric Auger
Instantiate and wire up the Memory Protection Controllers
in the MPS2 board itself.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
hw/arm/mps2-tz.c | 71 ++++++++++++++++++++++++++++++------------------
1 file changed, 44 insertions(+), 27 deletions(-)
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 8dc8bfd4ab2..a58b5dea79f 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -44,6 +44,7 @@
#include "hw/timer/cmsdk-apb-timer.h"
#include "hw/misc/mps2-scc.h"
#include "hw/misc/mps2-fpgaio.h"
+#include "hw/misc/tz-mpc.h"
#include "hw/arm/iotkit.h"
#include "hw/devices.h"
#include "net/net.h"
@@ -64,13 +65,12 @@ typedef struct {
IoTKit iotkit;
MemoryRegion psram;
- MemoryRegion ssram1;
+ MemoryRegion ssram[3];
MemoryRegion ssram1_m;
- MemoryRegion ssram23;
MPS2SCC scc;
MPS2FPGAIO fpgaio;
TZPPC ppc[5];
- UnimplementedDeviceState ssram_mpc[3];
+ TZMPC ssram_mpc[3];
UnimplementedDeviceState spi[5];
UnimplementedDeviceState i2c[4];
UnimplementedDeviceState i2s_audio;
@@ -95,16 +95,6 @@ typedef struct {
/* Main SYSCLK frequency in Hz */
#define SYSCLK_FRQ 20000000
-/* Initialize the auxiliary RAM region @mr and map it into
- * the memory map at @base.
- */
-static void make_ram(MemoryRegion *mr, const char *name,
- hwaddr base, hwaddr size)
-{
- memory_region_init_ram(mr, NULL, name, size, &error_fatal);
- memory_region_add_subregion(get_system_memory(), base, mr);
-}
-
/* Create an alias of an entire original MemoryRegion @orig
* located at @base in the memory map.
*/
@@ -224,6 +214,44 @@ static MemoryRegion *make_fpgaio(MPS2TZMachineState *mms, void *opaque,
return sysbus_mmio_get_region(SYS_BUS_DEVICE(fpgaio), 0);
}
+static MemoryRegion *make_mpc(MPS2TZMachineState *mms, void *opaque,
+ const char *name, hwaddr size)
+{
+ TZMPC *mpc = opaque;
+ int i = mpc - &mms->ssram_mpc[0];
+ MemoryRegion *ssram = &mms->ssram[i];
+ MemoryRegion *upstream;
+ char *mpcname = g_strdup_printf("%s-mpc", name);
+ static uint32_t ramsize[] = { 0x00400000, 0x00200000, 0x00200000 };
+ static uint32_t rambase[] = { 0x00000000, 0x28000000, 0x28200000 };
+
+ memory_region_init_ram(ssram, NULL, name, ramsize[i], &error_fatal);
+
+ init_sysbus_child(OBJECT(mms), mpcname, mpc,
+ sizeof(mms->ssram_mpc[0]), TYPE_TZ_MPC);
+ object_property_set_link(OBJECT(mpc), OBJECT(ssram),
+ "downstream", &error_fatal);
+ object_property_set_bool(OBJECT(mpc), true, "realized", &error_fatal);
+ /* Map the upstream end of the MPC into system memory */
+ upstream = sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 1);
+ memory_region_add_subregion(get_system_memory(), rambase[i], upstream);
+ /* and connect its interrupt to the IoTKit */
+ qdev_connect_gpio_out_named(DEVICE(mpc), "irq", 0,
+ qdev_get_gpio_in_named(DEVICE(&mms->iotkit),
+ "mpcexp_status", i));
+
+ /* The first SSRAM is a special case as it has an alias; accesses to
+ * the alias region at 0x00400000 must also go to the MPC upstream.
+ */
+ if (i == 0) {
+ make_ram_alias(&mms->ssram1_m, "mps.ssram1_m", upstream, 0x00400000);
+ }
+
+ g_free(mpcname);
+ /* Return the register interface MR for our caller to map behind the PPC */
+ return sysbus_mmio_get_region(SYS_BUS_DEVICE(mpc), 0);
+}
+
static void mps2tz_common_init(MachineState *machine)
{
MPS2TZMachineState *mms = MPS2TZ_MACHINE(machine);
@@ -285,14 +313,6 @@ static void mps2tz_common_init(MachineState *machine)
NULL, "mps.ram", 0x01000000);
memory_region_add_subregion(system_memory, 0x80000000, &mms->psram);
- /* The SSRAM memories should all be behind Memory Protection Controllers,
- * but we don't implement that yet.
- */
- make_ram(&mms->ssram1, "mps.ssram1", 0x00000000, 0x00400000);
- make_ram_alias(&mms->ssram1_m, "mps.ssram1_m", &mms->ssram1, 0x00400000);
-
- make_ram(&mms->ssram23, "mps.ssram23", 0x28000000, 0x00400000);
-
/* The overflow IRQs for all UARTs are ORed together.
* Tx, Rx and "combined" IRQs are sent to the NVIC separately.
* Create the OR gate for this.
@@ -322,12 +342,9 @@ static void mps2tz_common_init(MachineState *machine)
const PPCInfo ppcs[] = { {
.name = "apb_ppcexp0",
.ports = {
- { "ssram-mpc0", make_unimp_dev, &mms->ssram_mpc[0],
- 0x58007000, 0x1000 },
- { "ssram-mpc1", make_unimp_dev, &mms->ssram_mpc[1],
- 0x58008000, 0x1000 },
- { "ssram-mpc2", make_unimp_dev, &mms->ssram_mpc[2],
- 0x58009000, 0x1000 },
+ { "ssram-0", make_mpc, &mms->ssram_mpc[0], 0x58007000, 0x1000 },
+ { "ssram-1", make_mpc, &mms->ssram_mpc[1], 0x58008000, 0x1000 },
+ { "ssram-2", make_mpc, &mms->ssram_mpc[2], 0x58009000, 0x1000 },
},
}, {
.name = "apb_ppcexp1",
--
2.17.1
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (12 preceding siblings ...)
2018-06-04 15:29 ` [Qemu-devel] [PATCH v2 13/13] hw/arm/mps2-tz.c: Instantiate MPCs Peter Maydell
@ 2018-06-04 16:33 ` no-reply
2018-06-05 7:39 ` Peter Xu
2018-06-14 16:51 ` Peter Maydell
15 siblings, 0 replies; 40+ messages in thread
From: no-reply @ 2018-06-04 16:33 UTC (permalink / raw)
To: peter.maydell
Cc: famz, qemu-arm, qemu-devel, patches, peterx, eric.auger,
pbonzini, alex.bennee, rth
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180604152941.20374-1-peter.maydell@linaro.org
Subject: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
3b4bfa3eff hw/arm/mps2-tz.c: Instantiate MPCs
c2790822f9 hw/arm/iotkit: Wire up MPC interrupt lines
9746b202df hw/arm/iotkit: Instantiate MPC
4d48d06522 hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS
5e08d07dc5 hw/core/or-irq: Support more than 16 inputs to an OR gate
dff0cc68f1 hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate
cc42636d7c hw/misc/tz-mpc.c: Implement correct blocked-access behaviour
57b4be59f6 hw/misc/tz-mpc.c: Implement registers
cb3c936f45 hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller
7d9ec3d9dd exec.c: Handle IOMMUs in address_space_translate_for_iotlb()
3c592827bc iommu: Add IOMMU index argument to translate method
ca96fcdcf6 iommu: Add IOMMU index argument to notifier APIs
85bade9641 iommu: Add IOMMU index concept to IOMMU API
=== OUTPUT BEGIN ===
Checking PATCH 1/13: iommu: Add IOMMU index concept to IOMMU API...
Checking PATCH 2/13: iommu: Add IOMMU index argument to notifier APIs...
Checking PATCH 3/13: iommu: Add IOMMU index argument to translate method...
Checking PATCH 4/13: exec.c: Handle IOMMUs in address_space_translate_for_iotlb()...
Checking PATCH 5/13: hw/misc/tz-mpc.c: Implement the Arm TrustZone Memory Protection Controller...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#84:
new file mode 100644
total: 0 errors, 1 warnings, 486 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 6/13: hw/misc/tz-mpc.c: Implement registers...
Checking PATCH 7/13: hw/misc/tz-mpc.c: Implement correct blocked-access behaviour...
Checking PATCH 8/13: hw/misc/tz_mpc.c: Honour the BLK_LUT settings in translate...
Checking PATCH 9/13: hw/core/or-irq: Support more than 16 inputs to an OR gate...
ERROR: spaces required around that '*' (ctx:VxV)
#70: FILE: hw/core/or-irq.c:108:
+ .subsections = (const VMStateDescription*[]) {
^
total: 1 errors, 0 warnings, 62 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 10/13: hw/misc/iotkit-secctl.c: Implement SECMPCINTSTATUS...
ERROR: spaces required around that '*' (ctx:VxV)
#91: FILE: hw/misc/iotkit-secctl.c:711:
+ .subsections = (const VMStateDescription*[]) {
^
total: 1 errors, 0 warnings, 100 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 11/13: hw/arm/iotkit: Instantiate MPC...
Checking PATCH 12/13: hw/arm/iotkit: Wire up MPC interrupt lines...
Checking PATCH 13/13: hw/arm/mps2-tz.c: Instantiate MPCs...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (13 preceding siblings ...)
2018-06-04 16:33 ` [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC no-reply
@ 2018-06-05 7:39 ` Peter Xu
2018-06-05 9:13 ` Peter Maydell
2018-06-14 16:51 ` Peter Maydell
15 siblings, 1 reply; 40+ messages in thread
From: Peter Xu @ 2018-06-05 7:39 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, qemu-devel, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Eric Auger
On Mon, Jun 04, 2018 at 04:29:28PM +0100, Peter Maydell wrote:
> Hi; this is v2 of my iommu patchset, which does:
> * support IOMMUs that are aware of memory transaction attributes and
> may generate different translations for different attributes
> * support TCG execution out of memory which is behind an IOMMU
> * implement the Arm TrustZone Memory Protection Controller
> (which needs both the above features in the IOMMU core code)
> * use the MPC in the mps2-an505 board
>
> Patches 1-3 add the support for memory-transaction-aware
> IOMMUs. The general approach is that we have the concept of an
> IOMMU index (similar to the TCG MMU index), which selects which of
> multiple possible translation tables in the IOMMU we're trying to use.
> Most IOMMUs will support just a single index. When you register an
> IOMMU notifier and when you call the translate method you have to
> specify which IOMMU index you want. There's a method for getting the
> index that applies for a particular set of transaction attributes.
> All the current IOMMU implementations have just one iommu index, and
> all the current users of the notify API assume that.
Hi, Peter,
It seems that this series is still using the IOMMU index way. In case
I missed anything... Could you elaborate a bit on why this IOMMU index
solution is preferred comparing to the way to pass in MemTxAttrs? Or
was there any further discussion I missed on the topic?
My last post to previous series is here:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05702.html
In that, I was still confused on why we couldn't use the existing
MemTxAttrs directly instead of the new IOMMU index (and I explained on
why that was prefered at least to me). I didn't see replies
afterwards.
Frankly speaking I fully trust the expertise of you and all the
reviewers. I am just afraid I missed any context along the way.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
2018-06-05 7:39 ` Peter Xu
@ 2018-06-05 9:13 ` Peter Maydell
2018-06-05 13:25 ` Peter Xu
0 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-05 9:13 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-arm, QEMU Developers, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Eric Auger
On 5 June 2018 at 08:39, Peter Xu <peterx@redhat.com> wrote:
> On Mon, Jun 04, 2018 at 04:29:28PM +0100, Peter Maydell wrote:
>> Hi; this is v2 of my iommu patchset, which does:
>> * support IOMMUs that are aware of memory transaction attributes and
>> may generate different translations for different attributes
>> * support TCG execution out of memory which is behind an IOMMU
>> * implement the Arm TrustZone Memory Protection Controller
>> (which needs both the above features in the IOMMU core code)
>> * use the MPC in the mps2-an505 board
> It seems that this series is still using the IOMMU index way. In case
> I missed anything... Could you elaborate a bit on why this IOMMU index
> solution is preferred comparing to the way to pass in MemTxAttrs? Or
> was there any further discussion I missed on the topic?
>
> My last post to previous series is here:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05702.html
>
> In that, I was still confused on why we couldn't use the existing
> MemTxAttrs directly instead of the new IOMMU index (and I explained on
> why that was prefered at least to me). I didn't see replies
> afterwards.
Broadly speaking I didn't think I had any further better
explanation than I'd already given in that thread, eg here:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05250.html
and here:
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05513.html
If you want to make a specific (detailed) counterproposal of a
different API, I'm happy to look at whether that works for
the use cases I care about and whether it's a nicer way to do it.
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
2018-06-05 9:13 ` Peter Maydell
@ 2018-06-05 13:25 ` Peter Xu
0 siblings, 0 replies; 40+ messages in thread
From: Peter Xu @ 2018-06-05 13:25 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-arm, QEMU Developers, patches, Alex Bennée,
Richard Henderson, Paolo Bonzini, Eric Auger
On Tue, Jun 05, 2018 at 10:13:12AM +0100, Peter Maydell wrote:
> On 5 June 2018 at 08:39, Peter Xu <peterx@redhat.com> wrote:
> > On Mon, Jun 04, 2018 at 04:29:28PM +0100, Peter Maydell wrote:
> >> Hi; this is v2 of my iommu patchset, which does:
> >> * support IOMMUs that are aware of memory transaction attributes and
> >> may generate different translations for different attributes
> >> * support TCG execution out of memory which is behind an IOMMU
> >> * implement the Arm TrustZone Memory Protection Controller
> >> (which needs both the above features in the IOMMU core code)
> >> * use the MPC in the mps2-an505 board
>
> > It seems that this series is still using the IOMMU index way. In case
> > I missed anything... Could you elaborate a bit on why this IOMMU index
> > solution is preferred comparing to the way to pass in MemTxAttrs? Or
> > was there any further discussion I missed on the topic?
> >
> > My last post to previous series is here:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05702.html
> >
> > In that, I was still confused on why we couldn't use the existing
> > MemTxAttrs directly instead of the new IOMMU index (and I explained on
> > why that was prefered at least to me). I didn't see replies
> > afterwards.
>
> Broadly speaking I didn't think I had any further better
> explanation than I'd already given in that thread, eg here:
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05250.html
> and here:
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg05513.html
>
> If you want to make a specific (detailed) counterproposal of a
> different API, I'm happy to look at whether that works for
> the use cases I care about and whether it's a nicer way to do it.
I posted a few pesudo code (ok, it can actually compile...) to show
what I meant. Please have a look there:
[RFC 0/3] memory: enhance IOMMU notifier to support USER bit
I very suspect I missed some important requirement there but I cannot
really figure it out myself. Hope these patches can either provide an
alternative solution on the problem, or help me to figure out what I
missed. Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
2018-06-04 15:29 [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC Peter Maydell
` (14 preceding siblings ...)
2018-06-05 7:39 ` Peter Xu
@ 2018-06-14 16:51 ` Peter Maydell
2018-06-15 12:45 ` Peter Maydell
15 siblings, 1 reply; 40+ messages in thread
From: Peter Maydell @ 2018-06-14 16:51 UTC (permalink / raw)
To: qemu-arm, QEMU Developers
Cc: patches, Peter Xu, Eric Auger, Paolo Bonzini, Alex Bennée,
Richard Henderson
On 4 June 2018 at 16:29, Peter Maydell <peter.maydell@linaro.org> wrote:
> Hi; this is v2 of my iommu patchset, which does:
> * support IOMMUs that are aware of memory transaction attributes and
> may generate different translations for different attributes
> * support TCG execution out of memory which is behind an IOMMU
> * implement the Arm TrustZone Memory Protection Controller
> (which needs both the above features in the IOMMU core code)
> * use the MPC in the mps2-an505 board
> Unreviewed patches: 4, 6, 7, 8, 9, 10
Ping for further reviews, comments, etc, please. I'd like
to put this in via target-arm.next in the not too distant
future.
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/13] iommu: support txattrs, support TCG execution, implement TZ MPC
2018-06-14 16:51 ` Peter Maydell
@ 2018-06-15 12:45 ` Peter Maydell
0 siblings, 0 replies; 40+ messages in thread
From: Peter Maydell @ 2018-06-15 12:45 UTC (permalink / raw)
To: qemu-arm, QEMU Developers
Cc: patches, Peter Xu, Eric Auger, Paolo Bonzini, Alex Bennée,
Richard Henderson
On 14 June 2018 at 17:51, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 June 2018 at 16:29, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Hi; this is v2 of my iommu patchset, which does:
>> * support IOMMUs that are aware of memory transaction attributes and
>> may generate different translations for different attributes
>> * support TCG execution out of memory which is behind an IOMMU
>> * implement the Arm TrustZone Memory Protection Controller
>> (which needs both the above features in the IOMMU core code)
>> * use the MPC in the mps2-an505 board
>
>> Unreviewed patches: 4, 6, 7, 8, 9, 10
>
> Ping for further reviews, comments, etc, please. I'd like
> to put this in via target-arm.next in the not too distant
> future.
Thanks for the reviews; since 1-5 and 9 have been reviewed and
have no issues, I'm going to put those into target-arm.next
(to reduce the size of v3 of this series and avoid possible
issues with conflicts with other iommu related patchsets).
thanks
-- PMM
^ permalink raw reply [flat|nested] 40+ messages in thread