On Thu, 10 Jun 2021 17:38:02 +0100
Alexandru Elisei <alexandru.elisei@xxxxxxx> 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@xxxxxxx> 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.
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@xxxxxxx>
> >> ---
> >> 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);
> >> }
> >>
