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? 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. > +} (...) > +/** > + * 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/ > + * @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? > }