| Message ID | 20210816025915.213093-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw: ppc: sam460ex: Disable Ethernet devicetree nodes | expand |
On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: > IBM EMAC Ethernet controllers are not emulated by qemu. If they are > enabled in devicetree files, they are instantiated in Linux but > obviously won't work. Disable associated devicetree nodes to prevent > unpredictable behavior. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> I'll wait for Zoltan's opinion on this, but this sort of thing is why I was always pretty dubious about qemu *loading* a dtb file, rather than generating a dt internally. > --- > hw/ppc/sam460ex.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c > index 0737234d66..feb356e625 100644 > --- a/hw/ppc/sam460ex.c > +++ b/hw/ppc/sam460ex.c > @@ -194,6 +194,14 @@ static int sam460ex_load_device_tree(hwaddr addr, > _FDT(fdt_nop_node(fdt, offset)); > } > > + /* Ethernet interfaces are not emulated */ > + offset = fdt_node_offset_by_compatible(fdt, -1, "ibm,emac-460ex"); > + while (offset >= 0) { > + _FDT(fdt_setprop_string(fdt, offset, "status", "disabled")); > + offset = fdt_node_offset_by_compatible(fdt, offset, "ibm,emac-460ex"); > + } > + > + > /* set serial port clocks */ > offset = fdt_node_offset_by_compatible(fdt, -1, "ns16550"); > while (offset >= 0) {
On 8/16/21 7:41 AM, David Gibson wrote: > On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >> enabled in devicetree files, they are instantiated in Linux but >> obviously won't work. Disable associated devicetree nodes to prevent >> unpredictable behavior. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > I'll wait for Zoltan's opinion on this, but this sort of thing is why > I was always pretty dubious about qemu *loading* a dtb file, rather > than generating a dt internally. Hmm interesting point. >> --- >> hw/ppc/sam460ex.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >> index 0737234d66..feb356e625 100644 >> --- a/hw/ppc/sam460ex.c >> +++ b/hw/ppc/sam460ex.c >> @@ -194,6 +194,14 @@ static int sam460ex_load_device_tree(hwaddr addr, >> _FDT(fdt_nop_node(fdt, offset)); >> } >> >> + /* Ethernet interfaces are not emulated */ >> + offset = fdt_node_offset_by_compatible(fdt, -1, "ibm,emac-460ex"); >> + while (offset >= 0) { >> + _FDT(fdt_setprop_string(fdt, offset, "status", "disabled")); >> + offset = fdt_node_offset_by_compatible(fdt, offset, "ibm,emac-460ex"); >> + } Oh, I didn't know about appending 'status=disabled'. FWIW I'm carrying this patch to boot Linux on the raspi4 (but I prefer your way): -- >8 -- Author: Philippe Mathieu-Daudé <f4bug@amsat.org> Date: Sun Oct 18 22:39:19 2020 +0200 hw/arm/raspi: Remove unsupported raspi4 peripherals from device tree Kludge when using Linux kernels to reach userland. No device in DT -> no hardware initialization. Linux 5.9 uses the RPI_FIRMWARE_GET_CLOCKS so we now need to implement that feature too. Look like a cat and mouse game... Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index 6a793766840..93eb6591ee8 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -25,6 +25,7 @@ #include "hw/arm/boot.h" #include "sysemu/sysemu.h" #include "qom/object.h" +#include <libfdt.h> #define SMPBOOT_ADDR 0x300 /* this should leave enough space for ATAGS */ #define MVBAR_ADDR 0x400 /* secure vectors */ @@ -200,6 +201,29 @@ static void reset_secondary(ARMCPU *cpu, const struct arm_boot_info *info) cpu_set_pc(cs, info->smp_loader_start); } +static void raspi4_modify_dtb(const struct arm_boot_info *info, void *fdt) +{ + int offset; + + offset = fdt_node_offset_by_compatible(fdt, -1, "brcm,genet-v5"); + if (offset >= 0) { + /* FIXME we shouldn't nop the parent */ + offset = fdt_parent_offset(fdt, offset); + if (offset >= 0) { + if (!fdt_nop_node(fdt, offset)) { + warn_report("dtc: bcm2838-genet removed!"); + } + } + } + + offset = fdt_node_offset_by_compatible(fdt, -1, "brcm,avs-tmon-bcm2838"); + if (offset >= 0) { + if (!fdt_nop_node(fdt, offset)) { + warn_report("dtc: bcm2838-tmon removed!"); + } + } +} + static void setup_boot(MachineState *machine, RaspiProcessorId processor_id, size_t ram_size) { @@ -234,6 +258,9 @@ static void setup_boot(MachineState *machine, RaspiProcessorId processor_id, } s->binfo.secondary_cpu_reset_hook = reset_secondary; } + if (processor_id >= PROCESSOR_ID_BCM2838) { + s->binfo.modify_dtb = raspi4_modify_dtb; + } /* If the user specified a "firmware" image (e.g. UEFI), we bypass * the normal Linux boot process ---
On Mon, 16 Aug 2021, David Gibson wrote: > On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >> enabled in devicetree files, they are instantiated in Linux but >> obviously won't work. Disable associated devicetree nodes to prevent >> unpredictable behavior. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > I'll wait for Zoltan's opinion on this, but this sort of thing is why > I was always pretty dubious about qemu *loading* a dtb file, rather > than generating a dt internally. We are aiming to emulate the real SoC so we use the same dtb that belongs to that SoC instead of generating something similar but not quite the same. (QEMU also has a -dtb option but I'm not sure how many machines implement it.) So loading a dtb is not bad in my opinion. Given that we don't fully emulate every device in the SoC having devices described in the dtb that we don't have might cause warnings or errors from OSes that try to accesss these but that's all I've seen. I'm not sure what unpredictable behaviour could result apart from some log messages about missing ethernet so this should only be cosmetic to hide those errors. But other than that it likely should not break anything so I'm OK with this patch. (I did not implement ethernet ports becuase they are quite complex and we already have several PCI ethernet devices that work already with guests so it's easier to use those than spend time to implement another ethernet device.) Regards, BALATON Zoltan >> --- >> hw/ppc/sam460ex.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >> index 0737234d66..feb356e625 100644 >> --- a/hw/ppc/sam460ex.c >> +++ b/hw/ppc/sam460ex.c >> @@ -194,6 +194,14 @@ static int sam460ex_load_device_tree(hwaddr addr, >> _FDT(fdt_nop_node(fdt, offset)); >> } >> >> + /* Ethernet interfaces are not emulated */ >> + offset = fdt_node_offset_by_compatible(fdt, -1, "ibm,emac-460ex"); >> + while (offset >= 0) { >> + _FDT(fdt_setprop_string(fdt, offset, "status", "disabled")); >> + offset = fdt_node_offset_by_compatible(fdt, offset, "ibm,emac-460ex"); >> + } >> + >> + >> /* set serial port clocks */ >> offset = fdt_node_offset_by_compatible(fdt, -1, "ns16550"); >> while (offset >= 0) { > >
On Mon, 16 Aug 2021 at 06:46, David Gibson <david@gibson.dropbear.id.au> wrote: > > On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: > > IBM EMAC Ethernet controllers are not emulated by qemu. If they are > > enabled in devicetree files, they are instantiated in Linux but > > obviously won't work. Disable associated devicetree nodes to prevent > > unpredictable behavior. > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > I'll wait for Zoltan's opinion on this, but this sort of thing is why > I was always pretty dubious about qemu *loading* a dtb file, rather > than generating a dt internally. My take is that if you have to modify the dtb file to get QEMU to work, then that's a bug in QEMU that should be fixed. It's no worse than for the machines we have that don't use dtb and where the kernel just knows what the hardware is. (In my experience Arm kernels can be a bit finicky about wanting the right dtb and not some earlier or later one, so I think at least for Arm trying to generate a dt for our non-virt boards would have been pretty painful and liable to "new kernels don't boot any more" bugs.) Is it sufficient to create stub "unimplemented-device" ethernet controllers here, or does the guest want more behaviour than that? thanks -- PMM
On 8/16/21 12:26 PM, Peter Maydell wrote: > On Mon, 16 Aug 2021 at 06:46, David Gibson <david@gibson.dropbear.id.au> wrote: >> >> On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >>> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >>> enabled in devicetree files, they are instantiated in Linux but >>> obviously won't work. Disable associated devicetree nodes to prevent >>> unpredictable behavior. >>> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> >> I'll wait for Zoltan's opinion on this, but this sort of thing is why >> I was always pretty dubious about qemu *loading* a dtb file, rather >> than generating a dt internally. > > My take is that if you have to modify the dtb file to get QEMU to > work, then that's a bug in QEMU that should be fixed. It's no > worse than for the machines we have that don't use dtb and where > the kernel just knows what the hardware is. (In my experience > Arm kernels can be a bit finicky about wanting the right dtb > and not some earlier or later one, so I think at least for Arm > trying to generate a dt for our non-virt boards would have been > pretty painful and liable to "new kernels don't boot any more" bugs.) > > Is it sufficient to create stub "unimplemented-device" ethernet > controllers here, or does the guest want more behaviour than that? For raspi4 "unimplemented-device" is not enough, so what would be the ideal resolution for "the guest wants more behaviour" when we don't have time / interest / specs for the missing pieces?
On Mon, 16 Aug 2021, Philippe Mathieu-Daudé wrote: > On 8/16/21 12:26 PM, Peter Maydell wrote: >> On Mon, 16 Aug 2021 at 06:46, David Gibson <david@gibson.dropbear.id.au> wrote: >>> >>> On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >>>> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >>>> enabled in devicetree files, they are instantiated in Linux but >>>> obviously won't work. Disable associated devicetree nodes to prevent >>>> unpredictable behavior. >>>> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> >>> I'll wait for Zoltan's opinion on this, but this sort of thing is why >>> I was always pretty dubious about qemu *loading* a dtb file, rather >>> than generating a dt internally. >> >> My take is that if you have to modify the dtb file to get QEMU to >> work, then that's a bug in QEMU that should be fixed. It's no >> worse than for the machines we have that don't use dtb and where >> the kernel just knows what the hardware is. (In my experience >> Arm kernels can be a bit finicky about wanting the right dtb >> and not some earlier or later one, so I think at least for Arm >> trying to generate a dt for our non-virt boards would have been >> pretty painful and liable to "new kernels don't boot any more" bugs.) >> >> Is it sufficient to create stub "unimplemented-device" ethernet >> controllers here, or does the guest want more behaviour than that? > > For raspi4 "unimplemented-device" is not enough, so what would > be the ideal resolution for "the guest wants more behaviour" > when we don't have time / interest / specs for the missing > pieces? I guess ideal solution is to implement the missing device emulation, if we don't have resources for that effort then that's less than ideal but in that case patching the dtb to disable the device does not look too bad to me. I think that's an acceptable way to save the effort of implementing the device that's not srtictly needed. For sam460ex I think Linux has booted so far but displays an error about missing ethernet ports but this did not prevent it from booting. So unless recent kernels fail now, this patch is only suppresses those errors (and avoid going to code paths in guest kernel that probably never run on real hadware). I think there are arguments for and against it (the errors look ugly so we could prevent it but on the other hand then we have something different than on real hardware however missing the device means it's really different) so I'm not quite decided about either approach but I tend to try to keep it as similar to real hardware as possible (so using unmodified dtb) as long as it does not cause real problems. But if patching the dtb makes it nicer by avoiding a big and maybe confusing error message on boot and it does not break anything else then that's OK too. Regards, BALATON Zoltan
On 8/16/21 3:26 AM, Peter Maydell wrote: > On Mon, 16 Aug 2021 at 06:46, David Gibson <david@gibson.dropbear.id.au> wrote: >> >> On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >>> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >>> enabled in devicetree files, they are instantiated in Linux but >>> obviously won't work. Disable associated devicetree nodes to prevent >>> unpredictable behavior. >>> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> >> I'll wait for Zoltan's opinion on this, but this sort of thing is why >> I was always pretty dubious about qemu *loading* a dtb file, rather >> than generating a dt internally. > > My take is that if you have to modify the dtb file to get QEMU to > work, then that's a bug in QEMU that should be fixed. It's no > worse than for the machines we have that don't use dtb and where > the kernel just knows what the hardware is. (In my experience > Arm kernels can be a bit finicky about wanting the right dtb > and not some earlier or later one, so I think at least for Arm > trying to generate a dt for our non-virt boards would have been > pretty painful and liable to "new kernels don't boot any more" bugs.) > > Is it sufficient to create stub "unimplemented-device" ethernet > controllers here, or does the guest want more behaviour than that? > The controllers are instantiated (it looks like the Linux driver doesn't really check during probe if the hardware is present but relies on DT), but when trying to access them there is a PHY error. If a different Ethernet device is explicitly specified and instantiated, it either ends up as eth2 or as eth0, depending on the driver load order. That makes it difficult to test those other Ethernet devices (and with it the PCI controller). Plus, of course, there is always the danger of hitting a more severe problem. No problem though if this patch isn't accepted - I can carry it locally for my testing. I thought it would be acceptable because there is already other code doing the same, but I don't really depend on it. Thanks, Guenter
On Mon, 16 Aug 2021 at 15:00, Guenter Roeck <linux@roeck-us.net> wrote: > The controllers are instantiated (it looks like the Linux driver doesn't > really check during probe if the hardware is present but relies on DT), > but when trying to access them there is a PHY error. If a different Ethernet > device is explicitly specified and instantiated, it either ends up as eth2 > or as eth0, depending on the driver load order. That makes it difficult > to test those other Ethernet devices (and with it the PCI controller). I thought that code wasn't supposed to rely on the naming/ordering of ethX anyway these days? thanks -- PMM
On 8/16/21 7:03 AM, Peter Maydell wrote: > On Mon, 16 Aug 2021 at 15:00, Guenter Roeck <linux@roeck-us.net> wrote: >> The controllers are instantiated (it looks like the Linux driver doesn't >> really check during probe if the hardware is present but relies on DT), >> but when trying to access them there is a PHY error. If a different Ethernet >> device is explicitly specified and instantiated, it either ends up as eth2 >> or as eth0, depending on the driver load order. That makes it difficult >> to test those other Ethernet devices (and with it the PCI controller). > > I thought that code wasn't supposed to rely on the naming/ordering of > ethX anyway these days? > Depends on what you call "that code". I use small buildroot based root file systems for testing which do depend on the load order (or, rather, on eth0 working). Guenter
On Mon, 16 Aug 2021 at 15:11, Guenter Roeck <linux@roeck-us.net> wrote: > > On 8/16/21 7:03 AM, Peter Maydell wrote: > > On Mon, 16 Aug 2021 at 15:00, Guenter Roeck <linux@roeck-us.net> wrote: > >> The controllers are instantiated (it looks like the Linux driver doesn't > >> really check during probe if the hardware is present but relies on DT), > >> but when trying to access them there is a PHY error. If a different Ethernet > >> device is explicitly specified and instantiated, it either ends up as eth2 > >> or as eth0, depending on the driver load order. That makes it difficult > >> to test those other Ethernet devices (and with it the PCI controller). > > > > I thought that code wasn't supposed to rely on the naming/ordering of > > ethX anyway these days? > > > > Depends on what you call "that code". Sentence should be parsed 'I thought that (code wasn't supposed...)'. That is, my understanding was that the kernel simply doesn't provide the ordering/naming guarantee that your test code seems to want. -- PMM
On 8/16/21 7:19 AM, Peter Maydell wrote: > On Mon, 16 Aug 2021 at 15:11, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 8/16/21 7:03 AM, Peter Maydell wrote: >>> On Mon, 16 Aug 2021 at 15:00, Guenter Roeck <linux@roeck-us.net> wrote: >>>> The controllers are instantiated (it looks like the Linux driver doesn't >>>> really check during probe if the hardware is present but relies on DT), >>>> but when trying to access them there is a PHY error. If a different Ethernet >>>> device is explicitly specified and instantiated, it either ends up as eth2 >>>> or as eth0, depending on the driver load order. That makes it difficult >>>> to test those other Ethernet devices (and with it the PCI controller). >>> >>> I thought that code wasn't supposed to rely on the naming/ordering of >>> ethX anyway these days? >>> >> >> Depends on what you call "that code". > > Sentence should be parsed 'I thought that (code wasn't supposed...)'. > That is, my understanding was that the kernel simply doesn't provide > the ordering/naming guarantee that your test code seems to want. > Correct. However, as I said, I can live with carrying the patch locally. Thanks, Guenter
On 8/16/21 3:11 AM, Philippe Mathieu-Daudé wrote: > On 8/16/21 7:41 AM, David Gibson wrote: >> On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >>> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >>> enabled in devicetree files, they are instantiated in Linux but >>> obviously won't work. Disable associated devicetree nodes to prevent >>> unpredictable behavior. >>> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> >> I'll wait for Zoltan's opinion on this, but this sort of thing is why >> I was always pretty dubious about qemu *loading* a dtb file, rather >> than generating a dt internally. > > Hmm interesting point. > >>> --- >>> hw/ppc/sam460ex.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c >>> index 0737234d66..feb356e625 100644 >>> --- a/hw/ppc/sam460ex.c >>> +++ b/hw/ppc/sam460ex.c >>> @@ -194,6 +194,14 @@ static int sam460ex_load_device_tree(hwaddr addr, >>> _FDT(fdt_nop_node(fdt, offset)); >>> } >>> >>> + /* Ethernet interfaces are not emulated */ >>> + offset = fdt_node_offset_by_compatible(fdt, -1, "ibm,emac-460ex"); >>> + while (offset >= 0) { >>> + _FDT(fdt_setprop_string(fdt, offset, "status", "disabled")); >>> + offset = fdt_node_offset_by_compatible(fdt, offset, "ibm,emac-460ex"); >>> + } > > Oh, I didn't know about appending 'status=disabled'. > > FWIW I'm carrying this patch to boot Linux on the raspi4 > (but I prefer your way): > Neat, and excellent idea. I have a similar problem for xlnx-zcu102, where declaring the affected device as unsupported doesn't work either. I'll try the same trick there. Thanks! Guenter
On Mon, Aug 16, 2021 at 12:21:33PM +0200, BALATON Zoltan wrote: > On Mon, 16 Aug 2021, David Gibson wrote: > > On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: > > > IBM EMAC Ethernet controllers are not emulated by qemu. If they are > > > enabled in devicetree files, they are instantiated in Linux but > > > obviously won't work. Disable associated devicetree nodes to prevent > > > unpredictable behavior. > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > I'll wait for Zoltan's opinion on this, but this sort of thing is why > > I was always pretty dubious about qemu *loading* a dtb file, rather > > than generating a dt internally. > > We are aiming to emulate the real SoC so we use the same dtb that belongs to > that SoC instead of generating something similar but not quite the same. Well.. sure, but you don't *actually* emulate the real SoC, so you're advertising a dtb that doesn't match the real hardware, which is a bigger bug. > (QEMU also has a -dtb option but I'm not sure how many machines implement > it.) So loading a dtb is not bad in my opinion. Well.... I'm not all that convinced that -dtb is a good idea either. But to the extent that it is, I've assumed it's very much a "you must know what you're doing" option (like -bios) where it's the user's responsibility to make sure the dtb they're supplying matches the emulated hardware. > Given that we don't fully > emulate every device in the SoC having devices described in the dtb that we > don't have might cause warnings or errors from OSes that try to accesss > these but that's all I've seen. I'm not sure what unpredictable behaviour > could result apart from some log messages about missing ethernet so this > should only be cosmetic to hide those errors. But other than that it likely > should not break anything so I'm OK with this patch. (I did not implement > ethernet ports becuase they are quite complex and we already have several > PCI ethernet devices that work already with guests so it's easier to use > those than spend time to implement another ethernet device.) So, the thing I really dislike about this patch is that it's not committing to either approach. It's neither having a supplied dtb and making it qemu's job to match that behaviour exactly, nor is qemu supplying hardware and producing a dtb to describe that virtual hardware. It's doing a bit of both, which just seems like a recipe for confusion to me.
On Mon, Aug 16, 2021 at 01:58:08PM +0200, BALATON Zoltan wrote: > On Mon, 16 Aug 2021, Philippe Mathieu-Daudé wrote: > > On 8/16/21 12:26 PM, Peter Maydell wrote: > > > On Mon, 16 Aug 2021 at 06:46, David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: > > > > > IBM EMAC Ethernet controllers are not emulated by qemu. If they are > > > > > enabled in devicetree files, they are instantiated in Linux but > > > > > obviously won't work. Disable associated devicetree nodes to prevent > > > > > unpredictable behavior. > > > > > > > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > > > > > I'll wait for Zoltan's opinion on this, but this sort of thing is why > > > > I was always pretty dubious about qemu *loading* a dtb file, rather > > > > than generating a dt internally. > > > > > > My take is that if you have to modify the dtb file to get QEMU to > > > work, then that's a bug in QEMU that should be fixed. It's no > > > worse than for the machines we have that don't use dtb and where > > > the kernel just knows what the hardware is. (In my experience > > > Arm kernels can be a bit finicky about wanting the right dtb > > > and not some earlier or later one, so I think at least for Arm > > > trying to generate a dt for our non-virt boards would have been > > > pretty painful and liable to "new kernels don't boot any more" bugs.) > > > > > > Is it sufficient to create stub "unimplemented-device" ethernet > > > controllers here, or does the guest want more behaviour than that? > > > > For raspi4 "unimplemented-device" is not enough, so what would > > be the ideal resolution for "the guest wants more behaviour" > > when we don't have time / interest / specs for the missing > > pieces? > > I guess ideal solution is to implement the missing device emulation, if we > don't have resources for that effort then that's less than ideal but in that > case patching the dtb to disable the device does not look too bad to me. I > think that's an acceptable way to save the effort of implementing the device > that's not srtictly needed. I'm sympathetic to that, but in that case I think you shold drop the pretense of using this external dtb and matching it. Either generate the dtb in qemu, or snapshot the dtb, modify it to be the qemu-emulated version of the hardware and load that file explicitly. The second being basically just an easy way of generating a dtb when it's near-static. > For sam460ex I think Linux has booted so far but > displays an error about missing ethernet ports but this did not prevent it > from booting. So unless recent kernels fail now, this patch is only > suppresses those errors (and avoid going to code paths in guest kernel that > probably never run on real hadware). I think there are arguments for and > against it (the errors look ugly so we could prevent it but on the other > hand then we have something different than on real hardware however missing > the device means it's really different) so I'm not quite decided about > either approach but I tend to try to keep it as similar to real hardware as > possible (so using unmodified dtb) as long as it does not cause real > problems. But if patching the dtb makes it nicer by avoiding a big and maybe > confusing error message on boot and it does not break anything else then > that's OK too. > > Regards, > BALATON Zoltan
On Tue, 17 Aug 2021, David Gibson wrote: > On Mon, Aug 16, 2021 at 12:21:33PM +0200, BALATON Zoltan wrote: >> On Mon, 16 Aug 2021, David Gibson wrote: >>> On Sun, Aug 15, 2021 at 07:59:15PM -0700, Guenter Roeck wrote: >>>> IBM EMAC Ethernet controllers are not emulated by qemu. If they are >>>> enabled in devicetree files, they are instantiated in Linux but >>>> obviously won't work. Disable associated devicetree nodes to prevent >>>> unpredictable behavior. >>>> >>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> >>> I'll wait for Zoltan's opinion on this, but this sort of thing is why >>> I was always pretty dubious about qemu *loading* a dtb file, rather >>> than generating a dt internally. >> >> We are aiming to emulate the real SoC so we use the same dtb that belongs to >> that SoC instead of generating something similar but not quite the same. > > Well.. sure, but you don't *actually* emulate the real SoC, so you're > advertising a dtb that doesn't match the real hardware, which is a > bigger bug. > >> (QEMU also has a -dtb option but I'm not sure how many machines implement >> it.) So loading a dtb is not bad in my opinion. > > Well.... I'm not all that convinced that -dtb is a good idea either. > But to the extent that it is, I've assumed it's very much a "you must > know what you're doing" option (like -bios) where it's the user's > responsibility to make sure the dtb they're supplying matches the > emulated hardware. > >> Given that we don't fully >> emulate every device in the SoC having devices described in the dtb that we >> don't have might cause warnings or errors from OSes that try to accesss >> these but that's all I've seen. I'm not sure what unpredictable behaviour >> could result apart from some log messages about missing ethernet so this >> should only be cosmetic to hide those errors. But other than that it likely >> should not break anything so I'm OK with this patch. (I did not implement >> ethernet ports becuase they are quite complex and we already have several >> PCI ethernet devices that work already with guests so it's easier to use >> those than spend time to implement another ethernet device.) > > So, the thing I really dislike about this patch is that it's not > committing to either approach. It's neither having a supplied dtb and > making it qemu's job to match that behaviour exactly, nor is qemu > supplying hardware and producing a dtb to describe that virtual > hardware. It's doing a bit of both, which just seems like a recipe > for confusion to me. We could also modify the pc-bios/canyonlands.dts to comment out the ethernet ports from it or add the disabled properties there, maybe also adding a comment that explains these are not emulated in QEMU but to me keeping the dts unmodified, matching real hardware and let the board code patch it according to what's emulated looks more obvious to clearly show what changes we have from the originial hardware which would be less clear if we loaded a modified dtb. Modifying the dtb simplifies the board code but hides the differences from real hardware. So since we already have to modify the loaded dtb anyway I'm OK with changing it at the same place as this patch proposes. Regards, BALATON Zoltan
On Tue, 17 Aug 2021 at 10:25, BALATON Zoltan <balaton@eik.bme.hu> wrote: > We could also modify the pc-bios/canyonlands.dts to comment out the > ethernet ports from it or add the disabled properties there, maybe also > adding a comment that explains these are not emulated in QEMU but to me > keeping the dts unmodified, matching real hardware and let the board code > patch it according to what's emulated looks more obvious to clearly show > what changes we have from the originial hardware which would be less clear > if we loaded a modified dtb. Modifying the dtb simplifies the board code > but hides the differences from real hardware. So since we already have to > modify the loaded dtb anyway I'm OK with changing it at the same place as > this patch proposes. If this was preventing Linux from booting then I'd be a bit more inclined to it. But it doesn't sound like it's actually doing that? AIUI you just get a couple of non-functional ethernet interfaces that can be ignored, and "some devices don't actually work" is pretty much par-for-the-course for most QEMU models... -- PMM
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c index 0737234d66..feb356e625 100644 --- a/hw/ppc/sam460ex.c +++ b/hw/ppc/sam460ex.c @@ -194,6 +194,14 @@ static int sam460ex_load_device_tree(hwaddr addr, _FDT(fdt_nop_node(fdt, offset)); } + /* Ethernet interfaces are not emulated */ + offset = fdt_node_offset_by_compatible(fdt, -1, "ibm,emac-460ex"); + while (offset >= 0) { + _FDT(fdt_setprop_string(fdt, offset, "status", "disabled")); + offset = fdt_node_offset_by_compatible(fdt, offset, "ibm,emac-460ex"); + } + + /* set serial port clocks */ offset = fdt_node_offset_by_compatible(fdt, -1, "ns16550"); while (offset >= 0) {
IBM EMAC Ethernet controllers are not emulated by qemu. If they are enabled in devicetree files, they are instantiated in Linux but obviously won't work. Disable associated devicetree nodes to prevent unpredictable behavior. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- hw/ppc/sam460ex.c | 8 ++++++++ 1 file changed, 8 insertions(+)