diff mbox series

[RFC,03/14] hw/misc/bcm2835_property: Handle the 'domain state' property

Message ID 20190904171315.8354-4-f4bug@amsat.org (mailing list archive)
State New, archived
Headers show
Series hw/arm: Add the Raspberry Pi 4B | expand

Commit Message

Philippe Mathieu-Daudé Sept. 4, 2019, 5:13 p.m. UTC 
The kernel is happy with this change, so we don't need
to do anything more sophisticated.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/bcm2835_property.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Esteban Bosse Sept. 27, 2019, 8:51 p.m. UTC | #1 
Searching this property in the kernel, I found a lot of properties not
implemented.
https://github.com/raspberrypi/linux/blob/rpi-4.19.y/include/soc/bcm2835/raspberrypi-firmware.h#L41
Are the properties only added when they are necessaries for the standard
kernel use?

Reviewed-by: EstebanB

El mié., 4 sept. 2019 a las 19:13, Philippe Mathieu-Daudé (<f4bug@amsat.org>)
escribió:

> The kernel is happy with this change, so we don't need
> to do anything more sophisticated.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/misc/bcm2835_property.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index 399f0d9dd5..d8eb28f267 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -127,6 +127,14 @@ static void
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>              resplen = 8;
>              break;
>
> +        case 0x00030030: /* Get domain state */
> +            qemu_log_mask(LOG_UNIMP,
> +                          "bcm2835_property: 0x%08x get domain state
> NYI\n",
> +                          tag);
> +            /* FIXME returning uninitialized memory */
> +            resplen = 8;
> +            break;
> +
>          case 0x00038002: /* Set clock rate */
>          case 0x00038004: /* Set max clock rate */
>          case 0x00038007: /* Set min clock rate */
> --
> 2.20.1
>
>
Esteban Bosse Sept. 29, 2019, 7:01 a.m. UTC | #2 
El mié, 04-09-2019 a las 19:13 +0200, Philippe Mathieu-Daudé escribió:
> The kernel is happy with this change, so we don't need
> to do anything more sophisticated.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/misc/bcm2835_property.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index 399f0d9dd5..d8eb28f267 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -127,6 +127,14 @@ static void
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>              resplen = 8;
>              break;
>  
> +        case 0x00030030: /* Get domain state */
> +            qemu_log_mask(LOG_UNIMP,
> +                          "bcm2835_property: 0x%08x get domain state
> NYI\n",
> +                          tag);
> +            /* FIXME returning uninitialized memory */
> +            resplen = 8;
> +            break;
> +
>          case 0x00038002: /* Set clock rate */
>          case 0x00038004: /* Set max clock rate */
>          case 0x00038007: /* Set min clock rate */

Searching this property in the kernel, I found a lot of properties not
implemented. 
https://github.com/raspberrypi/linux/blob/rpi-4.19.y/include/soc/bcm2835/raspberrypi-firmware.h#L41
Are the properties only added when they are necessaries for the
standard kernel use?
Philippe Mathieu-Daudé Oct. 8, 2019, 9:32 a.m. UTC | #3 
On 9/29/19 9:01 AM, Esteban Bosse wrote:
> El mié, 04-09-2019 a las 19:13 +0200, Philippe Mathieu-Daudé escribió:
>> The kernel is happy with this change, so we don't need
>> to do anything more sophisticated.
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/misc/bcm2835_property.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
>> index 399f0d9dd5..d8eb28f267 100644
>> --- a/hw/misc/bcm2835_property.c
>> +++ b/hw/misc/bcm2835_property.c
>> @@ -127,6 +127,14 @@ static void
>> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>>               resplen = 8;
>>               break;
>>   
>> +        case 0x00030030: /* Get domain state */
>> +            qemu_log_mask(LOG_UNIMP,
>> +                          "bcm2835_property: 0x%08x get domain state
>> NYI\n",
>> +                          tag);
>> +            /* FIXME returning uninitialized memory */
>> +            resplen = 8;
>> +            break;
>> +
>>           case 0x00038002: /* Set clock rate */
>>           case 0x00038004: /* Set max clock rate */
>>           case 0x00038007: /* Set min clock rate */
> 
> Searching this property in the kernel, I found a lot of properties not
> implemented.
> https://github.com/raspberrypi/linux/blob/rpi-4.19.y/include/soc/bcm2835/raspberrypi-firmware.h#L41
> Are the properties only added when they are necessaries for the
> standard kernel use?

Yes, the idea is to emulate the bare minimum required to run your guest 
code. To test this series I used the closed source kernel provided by 
the Raspberry Pi foundation [1] which the one that does the most of 
UNIMP accesses, the Debian trunk one and I build one from [2].

The problem is old kernels don't support the raspi4, and recent kernels 
do a lot of property calls to use recent features.

A "property call" is a call from the ARM core to the VideoCore firmware, 
like a Remote Procedure Call. The ARM core write some 
arguments/structure to a memory space shared with the VideoCore, use a 
MailBox to send a 'property' key to the VideoCore and wait/poll for a 
response from the VC. The VC handles the 'property' call, if required 
fills the structure in the same memory space the ARM used, then reply 
via another MailBox channel to notify the ARM core.

QEMU does not model the VideoCore. Instead we directly fill the memory 
from the ARM property call. We fake the VC firmware.

[1] https://github.com/raspberrypi/firmware/blob/master/boot/kernel8.img
[2] https://github.com/raspberrypi/linux/tree/rpi-5.3.y
Esteban Bosse Oct. 24, 2019, 9:15 a.m. UTC | #4 
El mar, 08-10-2019 a las 11:32 +0200, Philippe Mathieu-Daudé escribió:
> On 9/29/19 9:01 AM, Esteban Bosse wrote:
> > El mié, 04-09-2019 a las 19:13 +0200, Philippe Mathieu-Daudé
> > escribió:
> > > The kernel is happy with this change, so we don't need
> > > to do anything more sophisticated.
> > > 
> > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > >   hw/misc/bcm2835_property.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/hw/misc/bcm2835_property.c
> > > b/hw/misc/bcm2835_property.c
> > > index 399f0d9dd5..d8eb28f267 100644
> > > --- a/hw/misc/bcm2835_property.c
> > > +++ b/hw/misc/bcm2835_property.c
> > > @@ -127,6 +127,14 @@ static void
> > > bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t
> > > value)
> > >               resplen = 8;
> > >               break;
> > >   
> > > +        case 0x00030030: /* Get domain state */
> > > +            qemu_log_mask(LOG_UNIMP,
> > > +                          "bcm2835_property: 0x%08x get domain
> > > state
> > > NYI\n",
> > > +                          tag);
> > > +            /* FIXME returning uninitialized memory */
> > > +            resplen = 8;
> > > +            break;
> > > +
> > >           case 0x00038002: /* Set clock rate */
> > >           case 0x00038004: /* Set max clock rate */
> > >           case 0x00038007: /* Set min clock rate */
> > 
> > Searching this property in the kernel, I found a lot of properties
> > not
> > implemented.
> > https://github.com/raspberrypi/linux/blob/rpi-4.19.y/include/soc/bcm2835/raspberrypi-firmware.h#L41
> > Are the properties only added when they are necessaries for the
> > standard kernel use?
> 
> Yes, the idea is to emulate the bare minimum required to run your
> guest 
> code. To test this series I used the closed source kernel provided
> by 
> the Raspberry Pi foundation [1] which the one that does the most of 
> UNIMP accesses, the Debian trunk one and I build one from [2].
> 
> The problem is old kernels don't support the raspi4, and recent
> kernels 
> do a lot of property calls to use recent features.
> 
> A "property call" is a call from the ARM core to the VideoCore
> firmware, 
> like a Remote Procedure Call. The ARM core write some 
> arguments/structure to a memory space shared with the VideoCore, use
> a 
> MailBox to send a 'property' key to the VideoCore and wait/poll for
> a 
> response from the VC. The VC handles the 'property' call, if
> required 
> fills the structure in the same memory space the ARM used, then
> reply 
> via another MailBox channel to notify the ARM core.
> 
> QEMU does not model the VideoCore. Instead we directly fill the
> memory 
> from the ARM property call. We fake the VC firmware.
> 
> [1] 
> https://github.com/raspberrypi/firmware/blob/master/boot/kernel8.img
> [2] https://github.com/raspberrypi/linux/tree/rpi-5.3.y
Thank you very much for all the data Phil.

Now I understood "the idea is to support the bare minimum for the
guest", I thought that the idea was a full HW support, but make sense
it's a lot of work to give full HW support.
diff mbox series

Patch

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 399f0d9dd5..d8eb28f267 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -127,6 +127,14 @@  static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 8;
             break;
 
+        case 0x00030030: /* Get domain state */
+            qemu_log_mask(LOG_UNIMP,
+                          "bcm2835_property: 0x%08x get domain state NYI\n",
+                          tag);
+            /* FIXME returning uninitialized memory */
+            resplen = 8;
+            break;
+
         case 0x00038002: /* Set clock rate */
         case 0x00038004: /* Set max clock rate */
         case 0x00038007: /* Set min clock rate */