在 2018/1/31 下午6:58, Cornelia Huck 写道:
On Tue, 30 Jan 2018 10:47:13 +0100
Yi Min Zhao <zyi...@linux.vnet.ibm.com> wrote:
Current s390x PCI IOMMU code is lack of flags' checking, including:
1) protection bit
2) table length
3) table offset
4) intermediate tables' invalid bit
5) format control bit
This patch introduces a new struct named S390IOTLBEntry, and makes up
these missed checkings. At the same time, inform the guest with the
corresponding error number when the check fails.
There are a lot of things in this patch I cannot review due to -ENODOC,
but some comments below.
Reviewed-by: Pierre Morel <pmo...@linux.vnet.ibm.com>
Signed-off-by: Yi Min Zhao <zyi...@linux.vnet.ibm.com>
---
hw/s390x/s390-pci-bus.c | 223 ++++++++++++++++++++++++++++++++++++++---------
hw/s390x/s390-pci-bus.h | 10 +++
hw/s390x/s390-pci-inst.c | 10 ---
3 files changed, 190 insertions(+), 53 deletions(-)
(...)
+/* ett is expected table type, -1 page table, 0 segment table, 1 region table
*/
+static uint64_t get_table_index(uint64_t iova, int8_t ett)
+{
+ switch (ett) {
+ case -1:
+ return calc_px(iova);
+ case 0:
+ return calc_sx(iova);
+ case 1:
+ return calc_rtx(iova);
+ }
+
+ return -1;
You use ett to differentiate between the three table types a lot. Is
this an architectured value, or an internal construct?
It's an architectured value to some degree, because it's used to descript
the translation more clearly in the doc.
If you introduced it yourself, it might make sense to switch to an enum
instead. Otherwise, using some #defines would improve readability of
the code.
OK. I will add macros in next version.
+}
(...)
+/**
+ * table_translate: do translation within one table and return the following
+ * table origin
+ *
+ * @entry: the entry being traslated, the result is stored in this.
s/traslated/translated/
OK.
+ * @to: the address of table origin.
+ * @ett: expected table type, 1 region table, 0 segment table and -1 page
table.
+ * @error: error code
+ */
+static uint64_t table_translate(S390IOTLBEntry *entry, uint64_t to, int8_t ett,
+ uint16_t *error)
(...)
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index be449210d9..63fa06fb97 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -644,16 +644,6 @@ int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t
r2, uintptr_t ra)
while (start < end) {
entry = imrc->translate(iommu_mr, start, IOMMU_NONE);
-
- if (!entry.translated_addr) {
- pbdev->state = ZPCI_FS_ERROR;
- setcc(cpu, ZPCI_PCI_LS_ERR);
- s390_set_status_code(env, r1, ZPCI_PCI_ST_INSUF_RES);
- s390_pci_generate_error_event(ERR_EVENT_SERR, pbdev->fh,
pbdev->fid,
- start, ERR_EVENT_Q_BIT);
- goto out;
- }
-
memory_region_notify_iommu(iommu_mr, entry);
start += entry.addr_mask + 1;
You're now progressing even though you might have generated an error
event. Is that what's intended?
Yes, this is wrong. The right thing is only delete the code generating
error event,
and keep the if check here in this patch.
}
