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

[Cornelia Huck <cohuck@redhat.com>]

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?

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?

>      }