Re: [Qemu-devel] [PATCH 1/3] s390x/pci: fixup the code walking IOMMU tables

[Yi Min Zhao <zyimin@linux.vnet.ibm.com>]

在 2018/1/31 下午6:58, Cornelia Huck 写道:
> On Tue, 30 Jan 2018 10:47:13 +0100
> Yi Min Zhao <zyimin@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 <pmorel@linux.vnet.ibm.com>
>> Signed-off-by: Yi Min Zhao <zyimin@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.
>
>>       }
>