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

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

On 10/2/26 8:43 PM, 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.
> 

Yes, but the original path was not passing an Error *errp back to a
caller, it was calling error_report (and then qemu_log_mask) directly;
your patch is changing this to a model where we return an Error *errp to
a caller.  Which in and of itself is totally fine, but we should make
efforts to then adhere to the rules laid out in include/qapi/error.h
where all failure paths should have an error when passing back an errp.

> If you believe a new message should be introduced, I can insert a commit
> for that - what would be the explanation?

Separate commit not needed, it only becomes necessary as part of this
change as described above.

As for a message, something like the following?

"ioat pba 0x%"PRIx64" pal 0x%"PRIx64" out of device dma range
[0x%"PRIx64" 0x%"PRIx64"]", pba, pal, pbdev->zpci_fn.sdma,
pbdev->zpci_fn.edma

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