Re: [PATCH v11 14/16] s390x/pci: Factor ioat sanity checks into a separate function

[Matthew Rosato <mjrosato@linux.ibm.com>]

> -static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
> -                    uintptr_t ra)
> +bool s390_pci_ioat_validate(S390PCIBusDevice *pbdev, uint64_t pba,
> +                             uint64_t pal, uint64_t g_iota, Error **errp)
>  {
> -    uint64_t pba = ldq_be_p(&fib.pba);
> -    uint64_t pal = ldq_be_p(&fib.pal);
> -    uint64_t g_iota = ldq_be_p(&fib.iota);
>      uint8_t dt = (g_iota >> 2) & 0x7;
>      bool t = s390_pci_is_translation_enabled(g_iota);
>  
> -    pba &= ~0xfff;
> -    pal |= 0xfff;
>      if (pba > pal || pba < pbdev->zpci_fn.sdma || pal > pbdev->zpci_fn.edma) {
> -        s390_program_interrupt(env, PGM_OPERAND, ra);
> -        return -EINVAL;
> +        return false;

This will now violate include/qapi/error.h because you're returning
false but not setting errp:

 * - On success, the function should not touch *errp.  On failure, it
 *   should set a new error, e.g. with error_setg(errp, ...), or
 *   propagate an existing one, e.g. with error_propagate(errp, ...).

A simple solution would be to add a new message for this case.

[...]
> +
> +static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
> +                    uintptr_t ra)
> +{
> +    Error *err = NULL;
> +    uint64_t pba = ldq_be_p(&fib.pba);
> +    uint64_t pal = ldq_be_p(&fib.pal);
> +    uint64_t g_iota = ldq_be_p(&fib.iota);
> +    bool t = s390_pci_is_translation_enabled(g_iota);
> +
> +    pba &= ~0xfff;
> +    pal |= 0xfff;
> +
> +    if (!s390_pci_ioat_validate(pbdev, pba, pal, g_iota, &err)) {
> +        if (err) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s\n", error_get_pretty(err));
> +            error_free(err);
> +        }

With that change, you won't need the if (err) check anymore.

Thanks,
Matt