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
