Re: [PATCH kvmtool 2/4] arm/fdt.c: Warn if MMIO device doesn't provide a node generator

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Andre,

On 6/14/21 3:07 PM, Andre Przywara wrote:
> On Thu, 10 Jun 2021 17:38:02 +0100
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi Alex,
>
>> On 6/10/21 5:13 PM, Andre Przywara wrote:
>>> On Wed,  9 Jun 2021 19:38:10 +0100
>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>  
>>>> Print a more helpful warning when a MMIO device hasn't set a function to
>>>> generate an FDT instead of causing a segmentation fault by dereferencing a
>>>> NULL pointer.  
>>> Not calling generate_mmio_fdt_nodes() if it's NULL is certainly a good
>>> idea, but how did you trigger it?  
>> I was able to trigger it when I was hacking a custom MMIO device emulation to test
>> some behaviour in KVM.
>>
>>> Because I am wondering whether every MMIO device needs to have an DT
>>> generator? And if that's not the case, a warning might be already too
>>> much.  
>> I don't know how the guest will be able to discover the device if it's not in the
>> DT, that's why I put the warning.
>> If there are devices which can be discovered by
>> the guest when they are missing from the DT, then I'll drop the warning.
> Well, not discovered, probably, but the guest (or parts of the guest,
> think EFI firmware) could have hard-coded knowledge about what to
> expect. That's what we did for the RTC and initially for the CFI flash.
> I agree it's not the best way to do (and we fixed both of those
> devices), but it's also nothing a *user* could do much about, so having a
> pr_debug() there seems more appropriate to me.

I think you're right, pr_debug() makes more sense here.

Thanks,

Alex

>
> Cheers,
> Andre
>
>>> So either just drop a print at all or use pr_info()/pr_debug()?
>>>
>>> Cheers,
>>> Andre
>>>  
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> ---
>>>>  arm/fdt.c | 7 ++++++-
>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arm/fdt.c b/arm/fdt.c
>>>> index 02091e9e0bee..06287a13e395 100644
>>>> --- a/arm/fdt.c
>>>> +++ b/arm/fdt.c
>>>> @@ -171,7 +171,12 @@ static int setup_fdt(struct kvm *kvm)
>>>>  	dev_hdr = device__first_dev(DEVICE_BUS_MMIO);
>>>>  	while (dev_hdr) {
>>>>  		generate_mmio_fdt_nodes = dev_hdr->data;
>>>> -		generate_mmio_fdt_nodes(fdt, dev_hdr, generate_irq_prop);
>>>> +		if (generate_mmio_fdt_nodes) {
>>>> +			generate_mmio_fdt_nodes(fdt, dev_hdr, generate_irq_prop);
>>>> +		} else {
>>>> +			pr_warning("Missing FDT node generator for MMIO device %d",
>>>> +				   dev_hdr->dev_num);
>>>> +		}
>>>>  		dev_hdr = device__next_dev(dev_hdr);
>>>>  	}
>>>>