On 8/26/26 4:49 PM, Matthew Rosato wrote:
> On 8/26/26 4:23 PM, Jared Rossi wrote:
>>
>>
>> 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?
>
> Since it's a switch statement looking at the type value would be
> redundant, but something like
> IPL_assert(false, "unexpected IPL type");
> should work
>
>>
>> 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.
>
> Yeah, my line of thinking was that if we shouldn't reach this code but
> somehow do, then something unexpected happened but we would kind of mask
> that by simply moving onto the next device.
>
>>
>> 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.
>>
> I leave it up to you and Joy to decide if you feel it makes sense to
> assert or update the message to be more accurate, but please do at least
> one of those.
>
> Outside of that, this patch LGTM.
>
> Thanks,
> Matt
Thanks for the suggestion!
Based on the discussion, I'd prefer having an IPL_assert/panic in the
default case and updating the subsequent error message to: "No valid
virtio-net device found." Since the default path should be unreachable,
reaching it would indicate an unexpected condition and should be treated
as fatal.
Please let me know if you'd prefer a different approach.
