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
>
