| Message ID | 20210609183812.29596-3-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm/arm64: PCI Express 1.1 support | expand |
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? 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. 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); > } >
Hi Andre, 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. Thanks, Alex > > 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); >> } >>
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. 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); > >> } > >>
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); >>>> } >>>>
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); }
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. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arm/fdt.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)