Re: [PATCH v1 1/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio net

[Jared Rossi <jrossi@linux.ibm.com>]

On 8/26/26 3:41 PM, Matthew Rosato wrote:
> On 8/26/26 3:30 PM, Jared Rossi wrote:
>>
>> On 8/26/26 1:33 PM, Matthew Rosato wrote:
>>>> +bool virtio_net_setup(void)
>>>> +{
>>>> +    switch (virtio_get_device()->ipl_type) {
>>>> +    case S390_IPL_TYPE_CCW:
>>>> +        return virtio_ccw_net_setup();
>>>> +    default:
>>>> +        return false;
>>>> +    }
>>>> +}
>>> This patch is largely renaming, but this does seem to have a subtle
>>> functional change right here.
>>>
>>> AFAICT before this patch attempting to netboot with anything other than
>>> a ccw device would hit
>>> IPL_assert(iplb.pbt == S390_IPL_TYPE_CCW, "IPL_TYPE_CCW expected");
>>>
>>> Now, we will never call virtio_ccw_net_setup(), return false and instead
>>> bail out with
>>> "No virtio net device found."
>>>
>>> That is new behavior for !IPL_TYPE_CCW after this patch.  For
>>> IPL_TYPE_PCI, patch 4 will change the behavior again.
>>>
>>> That's not a deal-breaker, but I do think it's worth a mention in the
>>> commit message.  I then also wonder if the message
>>> "No virtio net device found"
>>> would be more accurate if it instead read something like:
>>> "No supported virtio net device found"
>>>
>>> Thanks,
>>> Matt
>>>
>> I’m not sure this is a valid concern.  A non-ccw net device would be
>> rejected before getting to virtio_net_setup() earlier at the
>> find_boot_device() step either way.  In the case of virtio-net-pci
>> specifically, it would fail because VIRTIO_ID_NET is not a supported PCI
>> type yet.  For some sort of non-ccw non-pci netboot device, I believe there
>> wouldn't ever be an IPLB built for it, so it wouldn’t be recognized as
>> boot eligible at all.  I don’t think this patch affects any of that.
>>
>
> Based on that description it sounds like we don't even ever to expect to
> reach the new default: case then, as prior checks should have already
> weeded out all but the supported IPL_TYPE_* values.
>
> Should the default: case have an IPL_assert with its own message then?

Right, we shouldn’t ever hit the default case, but I don’t follow your
suggestion about an IPL_assert.  What condition might be asserted there?

In my opinion simply returning false is appropriate, which will just print
the error and move on to the next boot device if there one.  A full panic
could be justified since something weird would need to happen to get into
the default case at all, but I don’t think it’s really necessary.

If virtio_net_setup() does fail though, regardless of why, I do agree the
subsequent error message needs to be updated to clarify that no “valid” or
“supported” virtio network device was found rather than stating none was
found at all.

Regards,
Jared Rossi