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

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

On 10/5/26 9:23 AM, Konstantin Shkolnyy wrote:
> 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.
> 
The other 2 cases in this very function, where we are providing an error
message, are also due to invalid guest parameters.  We advertised the
necessary information to make a correct decision via CLP query payloads,
but the guest gave us bad input anyway.

The fact that these messages are in response to guest parameters is why
it made sense to switch to qemu_log_mask(LOG_GUEST_ERROR, ...) -- the
guest can control if the message pops by purposely setting bad inputs.

With that in mind, there's probably an argument for adding more messages
and/or trace events to other PGM_OPERAND paths for debug purposes since
these aren't things expected to fail on a normally-behaving guest.  But
that's beyond the scope of this series.

For this series:  you need to either add a message to this case or, if
you feel that strongly about not reporting a message for it, then
another solution would be to remove it from the s390_pci_ioat_validate()
function and check it in 2 places so that a message is not required and
then everything in s390_pci_ioat_validate() will generate an errp for
the false case, satisfying include/qapi/error.h.

Thanks,
Matt