[PATCH] hw/arm/bcm2835_property: Implement "get command line" message

[PATCH] hw/arm/bcm2835_property: Implement "get command line" message

[Daniel Bertalan]

This query copies the kernel command line into the message buffer. It
was previously stubbed out to return empty, this commit makes it reflect
the arguments specified with `-append`.

I observed the following peculiarities on my Pi 3B+:
- If the buffer is shorter than the string, the response header gives
  the full length, but no data is actually copied.
- No NUL terminator is added: even if the buffer is long enough to fit
  one, the buffer's original contents are preserved past the string's
  end.
- The VC firmware adds the following extra parameters beside the
  user-supplied ones (via /boot/cmdline.txt): `video`, `vc_mem.mem_base`
  and `vc_mem.mem_size`. This is currently not implemented in qemu.

Signed-off-by: Daniel Bertalan <dani@danielbertalan.dev>
---
 hw/arm/bcm2835_peripherals.c       | 2 ++
 hw/arm/bcm2836.c                   | 2 ++
 hw/arm/raspi.c                     | 2 ++
 hw/misc/bcm2835_property.c         | 7 ++++++-
 include/hw/misc/bcm2835_property.h | 1 +
 5 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index 3c2a4160cd..0233038b95 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -90,6 +90,8 @@ static void bcm2835_peripherals_init(Object *obj)
                             TYPE_BCM2835_PROPERTY);
     object_property_add_alias(obj, "board-rev", OBJECT(&s->property),
                               "board-rev");
+    object_property_add_alias(obj, "command-line", OBJECT(&s->property),
+                              "command-line");
 
     object_property_add_const_link(OBJECT(&s->property), "fb",
                                    OBJECT(&s->fb));
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index f894338fc6..166dc896c0 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -55,6 +55,8 @@ static void bcm2836_init(Object *obj)
                             TYPE_BCM2835_PERIPHERALS);
     object_property_add_alias(obj, "board-rev", OBJECT(&s->peripherals),
                               "board-rev");
+    object_property_add_alias(obj, "command-line", OBJECT(&s->peripherals),
+                              "command-line");
     object_property_add_alias(obj, "vcram-size", OBJECT(&s->peripherals),
                               "vcram-size");
 }
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 92d068d1f9..7b9221c924 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -280,6 +280,8 @@ static void raspi_machine_init(MachineState *machine)
     object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(machine->ram));
     object_property_set_int(OBJECT(&s->soc), "board-rev", board_rev,
                             &error_abort);
+    object_property_set_str(OBJECT(&s->soc), "command-line",
+                            machine->kernel_cmdline, &error_abort);
     qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     /* Create and plug in the SD cards */
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 890ae7bae5..225bce2ba4 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -282,7 +282,11 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
 
         case 0x00050001: /* Get command line */
-            resplen = 0;
+            resplen = strlen(s->command_line);
+            if (bufsize >= resplen)
+                address_space_write(&s->dma_as, value + 12,
+                                    MEMTXATTRS_UNSPECIFIED, s->command_line,
+                                    resplen);
             break;
 
         default:
@@ -413,6 +417,7 @@ static void bcm2835_property_realize(DeviceState *dev, Error **errp)
 
 static Property bcm2835_property_props[] = {
     DEFINE_PROP_UINT32("board-rev", BCM2835PropertyState, board_rev, 0),
+    DEFINE_PROP_STRING("command-line", BCM2835PropertyState, command_line),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/misc/bcm2835_property.h b/include/hw/misc/bcm2835_property.h
index 712b76b7a3..ba8896610c 100644
--- a/include/hw/misc/bcm2835_property.h
+++ b/include/hw/misc/bcm2835_property.h
@@ -30,6 +30,7 @@ struct BCM2835PropertyState {
     MACAddr macaddr;
     uint32_t board_rev;
     uint32_t addr;
+    char *command_line;
     bool pending;
 };
 
-- 
2.40.0

Re: [PATCH] hw/arm/bcm2835_property: Implement "get command line" message

[Peter Maydell]

On Tue, 25 Apr 2023 at 11:34, Daniel Bertalan <dani@danielbertalan.dev> wrote:
>
> This query copies the kernel command line into the message buffer. It
> was previously stubbed out to return empty, this commit makes it reflect
> the arguments specified with `-append`.
>
> I observed the following peculiarities on my Pi 3B+:
> - If the buffer is shorter than the string, the response header gives
>   the full length, but no data is actually copied.
> - No NUL terminator is added: even if the buffer is long enough to fit
>   one, the buffer's original contents are preserved past the string's
>   end.
> - The VC firmware adds the following extra parameters beside the
>   user-supplied ones (via /boot/cmdline.txt): `video`, `vc_mem.mem_base`
>   and `vc_mem.mem_size`. This is currently not implemented in qemu.

Are there any particularly interesting bits of guest software
that try to read this property ?

I added a brief comment to the code about the no-NUL-terminator
and short-buffer handling so that future readers of the code
don't have to refer back to the commit message:
+            /*
+             * We follow the firmware behaviour: no NUL terminator is
+             * written to the buffer, and if the buffer is too short
+             * we report the required length in the response header
+             * and copy nothing to the buffer.
+             */

and have applied this to target-arm.next; thanks.

-- PMM

Re: [PATCH] hw/arm/bcm2835_property: Implement "get command line" message

[Daniel Bertalan]

Hi Peter,

Thank you for merging the patch.

On Tuesday, May 2nd, 2023 at 12:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> Are there any particularly interesting bits of guest software
> that try to read this property ?

We plan to use it in the AArch64 port of SerenityOS temporarily, while
we are in the process of bringing up device tree support.
I added it here: <https://github.com/SerenityOS/serenity/pull/18557>

[sidenote]
By the way, do you know if anyone has managed to run the raspi3b emulation in
EL1 (under KVM/HVF)?

We are currently only targeting the Raspberry Pi as the the AArch64 porting effort
is a small subproject -- virtio peripheral support is out of the question
at the moment. It would be great if those of us that use AArch64 development
machines could run the OS under hardware acceleration, while still using the
existing Raspi kernel drivers.

I'd be happy to put in the work to make it possible, but I don't know where
to start.
[/sidenote]

I haven't checked any mainstream OSes if they use this message, sorry.

Regards,

Daniel

Re: [PATCH] hw/arm/bcm2835_property: Implement "get command line" message

[Peter Maydell]

On Tue, 2 May 2023 at 12:10, Daniel Bertalan <dani@danielbertalan.dev> wrote:
>
> Hi Peter,
>
> Thank you for merging the patch.
>
> On Tuesday, May 2nd, 2023 at 12:15, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Are there any particularly interesting bits of guest software
> > that try to read this property ?
>
> We plan to use it in the AArch64 port of SerenityOS temporarily, while
> we are in the process of bringing up device tree support.
> I added it here: <https://github.com/SerenityOS/serenity/pull/18557>
>
> [sidenote]
> By the way, do you know if anyone has managed to run the raspi3b emulation in
> EL1 (under KVM/HVF)?

No, that's not supported. The only board we have that works in
KVM is the 'virt' board. I guess in theory it could be bodged
into working, but AArch64 KVM isn't really intended as "run
an arbitrary guest kernel that uses an arbitrary machine and
set of devices", it's more "run a guest kernel that knows it's
running as a VM" (for instance there is no way to give the
guest something that says it's a Cortex-A53 -- you always get
the CPU type the host has).

-- PMM