|
From: | Yi Min Zhao |
Subject: | Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables |
Date: | Thu, 1 Feb 2018 19:28:02 +0800 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
在 2018/1/31 下午6:58, Cornelia Huck 写道:
On Tue, 30 Jan 2018 10:47:13 +0100 Yi Min Zhao <address@hidden> 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 <address@hidden> Signed-off-by: Yi Min Zhao <address@hidden> --- 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.
Yes, this is wrong. The right thing is only delete the code generating error event,+ * @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?
and keep the if check here in this patch.
}
[Prev in Thread] | Current Thread | [Next in Thread] |