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

[Konstantin Shkolnyy <kshk@linux.ibm.com>]

On 261002 19:43, Konstantin Shkolnyy wrote:
> On 261002 13:28, Matthew Rosato wrote:
>>
>>> -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.
> 
> But the original reg_ioat() also didn't print any message in this error 
> branch. I had to assume it was intentional, and preserved the behavior.
> 
> If you believe a new message should be introduced, I can insert a commit 
> for that - what would be the explanation?

Other code sites reporting PGM_OPERAND also don't generate error 
messages - it appears, the current policy is not to do that in the cases 
when the guest is at fault (gave invalid parameters). The case at hand 
falls in that category.

> 
>>
>> [...]
>>> +
>>> +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
>