[PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Guenter Roeck]

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(+)

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) {
-- 
2.25.1

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1562 bytes --]

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) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Philippe Mathieu-Daudé]

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
---

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Guenter Roeck]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[BALATON Zoltan]

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) {
>
>

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2698 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Peter Maydell]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Peter Maydell]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Philippe Mathieu-Daudé]

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?

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]

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


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Guenter Roeck]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Peter Maydell]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Guenter Roeck]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Peter Maydell]

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

Re: [PATCH] hw: ppc: sam460ex: Disable Ethernet devicetree nodes

[Guenter Roeck]

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