[Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

As the framebuffer settings are copied into the result message before it is
reconfigured, inconsistent behavior can happen when, for instance, you set with
a single message the width, height, and depth, and ask at the same time to
allocate the buffer and get the pitch and the size.

In this case, the reported pitch and size would be incorrect as they were
computed with the initial values of width, height and depth, not the ones the
client requested.

Signed-off-by: Sylvain Garrigues <sylvain@sylvaingarrigues.com>
Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 hw/misc/bcm2835_property.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 530411f..96e3b8f 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -21,6 +21,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     int n;
     uint32_t offset, length, color;
     uint32_t xres, yres, xoffset, yoffset, bpp, pixo, alpha;
+    uint32_t tmp_xres, tmp_yres, tmp_xoffset, tmp_yoffset, tmp_bpp, tmp_pixo, tmp_alpha;
     uint32_t *newxres = NULL, *newyres = NULL, *newxoffset = NULL,
         *newyoffset = NULL, *newbpp = NULL, *newpixo = NULL, *newalpha = NULL;
 
@@ -139,7 +140,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 
         case 0x00040001: /* Allocate buffer */
             stl_le_phys(&s->dma_as, value + 12, s->fbdev->base);
-            stl_le_phys(&s->dma_as, value + 16, s->fbdev->size);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
+            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres * tmp_bpp / 8);
             resplen = 8;
             break;
         case 0x00048001: /* Release buffer */
@@ -150,8 +154,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00040003: /* Get display width/height */
         case 0x00040004:
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->xres);
-            stl_le_phys(&s->dma_as, value + 16, s->fbdev->yres);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
+            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
+            stl_le_phys(&s->dma_as, value + 12, tmp_xres);
+            stl_le_phys(&s->dma_as, value + 16, tmp_yres);
             resplen = 8;
             break;
         case 0x00044003: /* Test display width/height */
@@ -167,7 +173,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 8;
             break;
         case 0x00040005: /* Get depth */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->bpp);
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 12, tmp_bpp);
             resplen = 4;
             break;
         case 0x00044005: /* Test depth */
@@ -179,7 +186,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040006: /* Get pixel order */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->pixo);
+            tmp_pixo = newpixo != NULL ? *newpixo : s->fbdev->pixo;
+            stl_le_phys(&s->dma_as, value + 12, tmp_pixo);
             resplen = 4;
             break;
         case 0x00044006: /* Test pixel order */
@@ -191,7 +199,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040007: /* Get alpha */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->alpha);
+            tmp_alpha = newalpha != NULL ? *newalpha : s->fbdev->alpha;
+            stl_le_phys(&s->dma_as, value + 12, tmp_alpha);
             resplen = 4;
             break;
         case 0x00044007: /* Test pixel alpha */
@@ -203,12 +212,16 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040008: /* Get pitch */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->pitch);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 12, tmp_xres * tmp_bpp / 8);
             resplen = 4;
             break;
         case 0x00040009: /* Get virtual offset */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->xoffset);
-            stl_le_phys(&s->dma_as, value + 16, s->fbdev->yoffset);
+            tmp_xoffset = newxoffset != NULL ? *newxoffset : s->fbdev->xoffset;
+            tmp_yoffset = newyoffset != NULL ? *newyoffset : s->fbdev->yoffset;
+            stl_le_phys(&s->dma_as, value + 12, tmp_xoffset);
+            stl_le_phys(&s->dma_as, value + 16, tmp_yoffset);
             resplen = 8;
             break;
         case 0x00044009: /* Test virtual offset */
-- 
2.8.1

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

Hello,

Shall we commit this patch?

Very best,
Sylvain


> Le 22 avr. 2016 à 14:42, Sylvain Garrigues <sylvain@sylvaingarrigues.com> a écrit :
> 
> As the framebuffer settings are copied into the result message before it is
> reconfigured, inconsistent behavior can happen when, for instance, you set with
> a single message the width, height, and depth, and ask at the same time to
> allocate the buffer and get the pitch and the size.
> 
> In this case, the reported pitch and size would be incorrect as they were
> computed with the initial values of width, height and depth, not the ones the
> client requested.
> 
> Signed-off-by: Sylvain Garrigues <sylvain@sylvaingarrigues.com>
> Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
> hw/misc/bcm2835_property.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index 530411f..96e3b8f 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -21,6 +21,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>     int n;
>     uint32_t offset, length, color;
>     uint32_t xres, yres, xoffset, yoffset, bpp, pixo, alpha;
> +    uint32_t tmp_xres, tmp_yres, tmp_xoffset, tmp_yoffset, tmp_bpp, tmp_pixo, tmp_alpha;
>     uint32_t *newxres = NULL, *newyres = NULL, *newxoffset = NULL,
>         *newyoffset = NULL, *newbpp = NULL, *newpixo = NULL, *newalpha = NULL;
> 
> @@ -139,7 +140,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
> 
>         case 0x00040001: /* Allocate buffer */
>             stl_le_phys(&s->dma_as, value + 12, s->fbdev->base);
> -            stl_le_phys(&s->dma_as, value + 16, s->fbdev->size);
> +            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
> +            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
> +            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
> +            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres * tmp_bpp / 8);
>             resplen = 8;
>             break;
>         case 0x00048001: /* Release buffer */
> @@ -150,8 +154,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>             break;
>         case 0x00040003: /* Get display width/height */
>         case 0x00040004:
> -            stl_le_phys(&s->dma_as, value + 12, s->fbdev->xres);
> -            stl_le_phys(&s->dma_as, value + 16, s->fbdev->yres);
> +            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
> +            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
> +            stl_le_phys(&s->dma_as, value + 12, tmp_xres);
> +            stl_le_phys(&s->dma_as, value + 16, tmp_yres);
>             resplen = 8;
>             break;
>         case 0x00044003: /* Test display width/height */
> @@ -167,7 +173,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>             resplen = 8;
>             break;
>         case 0x00040005: /* Get depth */
> -            stl_le_phys(&s->dma_as, value + 12, s->fbdev->bpp);
> +            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
> +            stl_le_phys(&s->dma_as, value + 12, tmp_bpp);
>             resplen = 4;
>             break;
>         case 0x00044005: /* Test depth */
> @@ -179,7 +186,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>             resplen = 4;
>             break;
>         case 0x00040006: /* Get pixel order */
> -            stl_le_phys(&s->dma_as, value + 12, s->fbdev->pixo);
> +            tmp_pixo = newpixo != NULL ? *newpixo : s->fbdev->pixo;
> +            stl_le_phys(&s->dma_as, value + 12, tmp_pixo);
>             resplen = 4;
>             break;
>         case 0x00044006: /* Test pixel order */
> @@ -191,7 +199,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>             resplen = 4;
>             break;
>         case 0x00040007: /* Get alpha */
> -            stl_le_phys(&s->dma_as, value + 12, s->fbdev->alpha);
> +            tmp_alpha = newalpha != NULL ? *newalpha : s->fbdev->alpha;
> +            stl_le_phys(&s->dma_as, value + 12, tmp_alpha);
>             resplen = 4;
>             break;
>         case 0x00044007: /* Test pixel alpha */
> @@ -203,12 +212,16 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>             resplen = 4;
>             break;
>         case 0x00040008: /* Get pitch */
> -            stl_le_phys(&s->dma_as, value + 12, s->fbdev->pitch);
> +            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
> +            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
> +            stl_le_phys(&s->dma_as, value + 12, tmp_xres * tmp_bpp / 8);
>             resplen = 4;
>             break;
>         case 0x00040009: /* Get virtual offset */
> -            stl_le_phys(&s->dma_as, value + 12, s->fbdev->xoffset);
> -            stl_le_phys(&s->dma_as, value + 16, s->fbdev->yoffset);
> +            tmp_xoffset = newxoffset != NULL ? *newxoffset : s->fbdev->xoffset;
> +            tmp_yoffset = newyoffset != NULL ? *newyoffset : s->fbdev->yoffset;
> +            stl_le_phys(&s->dma_as, value + 12, tmp_xoffset);
> +            stl_le_phys(&s->dma_as, value + 16, tmp_yoffset);
>             resplen = 8;
>             break;
>         case 0x00044009: /* Test virtual offset */
> -- 
> 2.8.1
> 

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Peter Maydell]

On 2 May 2016 at 22:21, Sylvain Garrigues <sylgar@gmail.com> wrote:
> Hello,
>
> Shall we commit this patch?

Hi; this is on my list of patches to deal with, but QEMU
is still in code freeze for the upcoming 2.6 release.
When we reopen the tree after release it will go in
shortly after that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Peter Maydell]

On 22 April 2016 at 12:42, Sylvain Garrigues
<sylvain@sylvaingarrigues.com> wrote:
> As the framebuffer settings are copied into the result message before it is
> reconfigured, inconsistent behavior can happen when, for instance, you set with
> a single message the width, height, and depth, and ask at the same time to
> allocate the buffer and get the pitch and the size.
>
> In this case, the reported pitch and size would be incorrect as they were
> computed with the initial values of width, height and depth, not the ones the
> client requested.
>
> Signed-off-by: Sylvain Garrigues <sylvain@sylvaingarrigues.com>
> Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
>  hw/misc/bcm2835_property.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)

Applied to target-arm.next, thanks. (I folded a couple of long lines
that checkpatch.pl complains about.)

-- PMM

[Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

As the framebuffer settings are copied into the result message before it is reconfigured, inconsistent behavior can happen when, for instance, you set with a sinle message the width, height, and depth, and ask at the same time to allocate the buffer and get the pitch and the size.

In this case, the reported pitch and size would be incorrect as they were computed with the initial values of width, height and depth, not the ones the client requested.

Signed-off-by: Sylvain Garrigues <sylvain@sylvaingarrigues.com>
---
 hw/misc/bcm2835_property.c | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 530411f..0102144 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -21,6 +21,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
     int n;
     uint32_t offset, length, color;
     uint32_t xres, yres, xoffset, yoffset, bpp, pixo, alpha;
+    uint32_t tmp_xres, tmp_yres, tmp_xoffset, tmp_yoffset, tmp_bpp, tmp_pixo, tmp_alpha;
     uint32_t *newxres = NULL, *newyres = NULL, *newxoffset = NULL,
         *newyoffset = NULL, *newbpp = NULL, *newpixo = NULL, *newalpha = NULL;
 
@@ -139,7 +140,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
 
         case 0x00040001: /* Allocate buffer */
             stl_le_phys(&s->dma_as, value + 12, s->fbdev->base);
-            stl_le_phys(&s->dma_as, value + 16, s->fbdev->size);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
+            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres * (tmp_bpp >> 3));
             resplen = 8;
             break;
         case 0x00048001: /* Release buffer */
@@ -150,8 +154,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             break;
         case 0x00040003: /* Get display width/height */
         case 0x00040004:
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->xres);
-            stl_le_phys(&s->dma_as, value + 16, s->fbdev->yres);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
+            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
+            stl_le_phys(&s->dma_as, value + 12, tmp_xres);
+            stl_le_phys(&s->dma_as, value + 16, tmp_yres);
             resplen = 8;
             break;
         case 0x00044003: /* Test display width/height */
@@ -167,7 +173,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 8;
             break;
         case 0x00040005: /* Get depth */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->bpp);
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 12, tmp_bpp);
             resplen = 4;
             break;
         case 0x00044005: /* Test depth */
@@ -179,7 +186,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040006: /* Get pixel order */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->pixo);
+            tmp_pixo = newpixo != NULL ? *newpixo : s->fbdev->pixo;
+            stl_le_phys(&s->dma_as, value + 12, tmp_pixo);
             resplen = 4;
             break;
         case 0x00044006: /* Test pixel order */
@@ -191,7 +199,8 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040007: /* Get alpha */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->alpha);
+            tmp_alpha = newalpha != NULL ? *newalpha : s->fbdev->alpha;
+            stl_le_phys(&s->dma_as, value + 12, tmp_alpha);
             resplen = 4;
             break;
         case 0x00044007: /* Test pixel alpha */
@@ -203,12 +212,16 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
             resplen = 4;
             break;
         case 0x00040008: /* Get pitch */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->pitch);
+            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
+            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
+            stl_le_phys(&s->dma_as, value + 12, tmp_xres * (tmp_bpp >> 3));
             resplen = 4;
             break;
         case 0x00040009: /* Get virtual offset */
-            stl_le_phys(&s->dma_as, value + 12, s->fbdev->xoffset);
-            stl_le_phys(&s->dma_as, value + 16, s->fbdev->yoffset);
+            tmp_xoffset = newxoffset != NULL ? *newxoffset : s->fbdev->xoffset;
+            tmp_yoffset = newyoffset != NULL ? *newyoffset : s->fbdev->yoffset;
+            stl_le_phys(&s->dma_as, value + 12, tmp_xoffset);
+            stl_le_phys(&s->dma_as, value + 16, tmp_yoffset);
             resplen = 8;
             break;
         case 0x00044009: /* Test virtual offset */
-- 
2.8.1

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Peter Maydell]

On 21 April 2016 at 11:41, Sylvain Garrigues
<sylvain@sylvaingarrigues.com> wrote:
> As the framebuffer settings are copied into the result message before
> it is reconfigured, inconsistent behavior can happen when, for instance,
> you set with a sinle message the width, height, and depth, and ask at
> the same time to allocate the buffer and get the pitch and the size.
>
> In this case, the reported pitch and size would be incorrect as they
> were computed with the initial values of width, height and depth, not
> the ones the client requested.

Thanks for this patch. I think at this point in the 2.6 release cycle
we should leave this for the 2.7 release, given that it doesn't affect
the booting of the older Linux and Windows versions that work on the raspi2
board.

I'll let Andrew review the patch...

-- PMM

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

It does prevent FreeBSD to boot correctly.

With that patch and 
#define KERNEL_LOAD_ADDR 0x00200000
in arm/boot/boot.c, official FreeBSD RPI2 images boot like a charm :-)

BTW, would be great to be able to set the load addr on the command line, wouldn’t it?


> Le 21 avr. 2016 à 14:45, Peter Maydell <peter.maydell@linaro.org> a écrit :
> 
> On 21 April 2016 at 11:41, Sylvain Garrigues
> <sylvain@sylvaingarrigues.com> wrote:
>> As the framebuffer settings are copied into the result message before
>> it is reconfigured, inconsistent behavior can happen when, for instance,
>> you set with a sinle message the width, height, and depth, and ask at
>> the same time to allocate the buffer and get the pitch and the size.
>> 
>> In this case, the reported pitch and size would be incorrect as they
>> were computed with the initial values of width, height and depth, not
>> the ones the client requested.
> 
> Thanks for this patch. I think at this point in the 2.6 release cycle
> we should leave this for the 2.7 release, given that it doesn't affect
> the booting of the older Linux and Windows versions that work on the raspi2
> board.
> 
> I'll let Andrew review the patch...
> 
> -- PMM

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Peter Maydell]

On 21 April 2016 at 13:50, Sylvain Garrigues
<sylvain@sylvaingarrigues.com> wrote:
> It does prevent FreeBSD to boot correctly.
>
> With that patch and
> #define KERNEL_LOAD_ADDR 0x00200000
> in arm/boot/boot.c, official FreeBSD RPI2 images boot like a charm :-)

This is the kind of thing that is useful to mention in the commit
message, because it tells people why the change is important...

> BTW, would be great to be able to set the load addr on the command
> line, wouldn’t it?

The booting protocol does not mandate any particular address
that the kernel has to be loaded at:
https://www.kernel.org/doc/Documentation/arm/Booting

If the FreeBSD kernel can't handle being loaded wherever
we put it, that sounds like a FreeBSD bug to me...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

Le 21 avr. 2016 à 14:54, Peter Maydell <peter.maydell@linaro.org> a écrit :
> On 21 April 2016 at 13:50, Sylvain Garrigues
> <sylvain@sylvaingarrigues.com> wrote:
>> It does prevent FreeBSD to boot correctly.
>> 
>> With that patch and
>> #define KERNEL_LOAD_ADDR 0x00200000
>> in arm/boot/boot.c, official FreeBSD RPI2 images boot like a charm :-)
> 
> This is the kind of thing that is useful to mention in the commit
> message, because it tells people why the change is important…

You are obviously right, sorry about that.

>> BTW, would be great to be able to set the load addr on the command
>> line, wouldn’t it?
> 
> The booting protocol does not mandate any particular address
> that the kernel has to be loaded at:
> https://www.kernel.org/doc/Documentation/arm/Booting

I didn’t know qemu would blindly follow and stick to Linux rules, but I understand. I use qemu for baremetal and *BSD emulation, I wish it would be easier to change the loading address (and not stick with Linux rules like so many people again) :-)  


> If the FreeBSD kernel can't handle being loaded wherever
> we put it, that sounds like a FreeBSD bug to me…

I would not say it is a bug, but an explicit assumption. Indeed, the kernel expects it is loaded on a 2 MB page boundary.
I’m no expert, but I believe it was made so that we could remove the need for setting the physical address in the building process. The virtual to physical offset is computed more simply with that assumption. 

With the patch you committed for me earlier today (b4850e5), the patch we are talking about in this email (which is not a feature but a bug fix in my opinion), and the ability to set the load address to 0x200000 (on the command line if possible), the following line just works straight to the login prompt :-) 
# ./arm-softmmu/qemu-system-arm -M raspi2 -m 1024 -smp 4 -kernel kernel.bin -serial stdio -dtb rpi2.dtb -sd FreeBSD-11.0-CURRENT-arm-armv6-RPI2-20160408-r297692.img

FreeBSD-11.0-CURRENT-arm-armv6-RPI2-20160408-r297692.img is the latest FreeBSD official image.
rpi2.dtb and kernel.bin are in the /boot/ partition of that img file. 

That’s it for my patches to make qemu and BSDs the best lovers in the world.

Have a good day,
Sylvain Garrigues.

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Peter Maydell]

On 21 April 2016 at 14:15, Sylvain Garrigues
<sylvain@sylvaingarrigues.com> wrote:
> Le 21 avr. 2016 à 14:54, Peter Maydell <peter.maydell@linaro.org> a écrit :
>> On 21 April 2016 at 13:50, Sylvain Garrigues
>> <sylvain@sylvaingarrigues.com> wrote:
>>> BTW, would be great to be able to set the load addr on the command
>>> line, wouldn’t it?
>>
>> The booting protocol does not mandate any particular address
>> that the kernel has to be loaded at:
>> https://www.kernel.org/doc/Documentation/arm/Booting
>
> I didn’t know qemu would blindly follow and stick to Linux rules,
> but I understand. I use qemu for baremetal and *BSD emulation, I wish
> it would be easier to change the loading address (and not stick with
> Linux rules like so many people again) :-)

-kernel for a non-ELF file is specifically "boot a Linux kernel according
to the Linux kernel booting protocol". Your other options for booting
are (1) provide -kernel with an ELF file, which we will load at whatever
start address the ELF header specifies, or (2) on boards which have a
flash device, use -bios or -pflash options to specify a flash image to
go in that flash device (I don't think raspi2 has this yet though).
For bare metal testing either of those last two seem like a better
approach; you should also be able to wrap the FreeBSD kernel in an
ELF file to get it to work like that (you'll need to handle SMP
secondary boot yourself though; or just stick a relocation shim
on the front of the FreeBSD kernel).

The reason I'm a bit reluctant to add extra bells and whistles to
the builtin bootloader is that it can quickly get out of hand:
should we support all of Linux, FreeBSD, NetBSD, OpenBSD, Windows,
etc etc etc? At some point the right answer becomes "run the real
guest bootloader like u-boot from flash and let that start the
kernel".

There may be something we can do here to make FreeBSD's life
easier, but we definitely can't do it on the eve of a release.

>> If the FreeBSD kernel can't handle being loaded wherever
>> we put it, that sounds like a FreeBSD bug to me…
>
> I would not say it is a bug, but an explicit assumption. Indeed, the
> kernel expects it is loaded on a 2 MB page boundary.
> I’m no expert, but I believe it was made so that we could remove the
> need for setting the physical address in the building process. The
> virtual to physical offset is computed more simply with that assumption.

> With the patch you committed for me earlier today (b4850e5), the patch
> we are talking about in this email (which is not a feature but a bug
> fix in my opinion)

FWIW, I agree this is a bug fix; it's just that we're hours away from
tagging the final RC for this release cycle, so the bar for "is this
bug important enough to put in now?" is quite high...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

Le 21 avr. 2016 à 15:42, Peter Maydell <peter.maydell@linaro.org> a écrit :
> 
> There may be something we can do here to make FreeBSD's life
> easier, but we definitely can't do it on the eve of a release.

I didn’t know it was release day, my timing is not perfect then, sorry about that, I didn’t intend to put stress on you guys today.
Like you mentioned, the Linux boot protocol doesn’t mandate any loading address, hence the possibility to set it on the command line. 
It would benefit not only to FreeBSD (which is strictly Linux boot ABI compliant BTW - that is how I found the qemu bootloader bug and fixed it in b4850e5) but all other OS.
On the real hardware Raspberry Pi, there is the kernel_address firmware feature which enable to set the kernel load address. Would be neat to have it *someday* in qemu for any board if it is not too hard to implement.

> 
> FWIW, I agree this is a bug fix; it's just that we're hours away from
> tagging the final RC for this release cycle, so the bar for "is this
> bug important enough to put in now?" is quite high…

Again, I didn’t know about the timing. Indeed it is a bug but I let the team decide about including it in this release. Personally, I don’t care :-) (apart from my own ego being able to say « I contributed to making this release compatible with FreeBSD »)

Cheers,
Sylvain.

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Andrew Baumann]

> From: Qemu-devel [mailto:qemu-devel-
> bounces+andrew.baumann=microsoft.com@nongnu.org] On Behalf Of Sylvain
> Garrigues
> Sent: Thursday, 21 April 2016 12:42
> 
> As the framebuffer settings are copied into the result message before it is
> reconfigured, inconsistent behavior can happen when, for instance, you set
> with a sinle message the width, height, and depth, and ask at the same time to
> allocate the buffer and get the pitch and the size.
> 
> In this case, the reported pitch and size would be incorrect as they were
> computed with the initial values of width, height and depth, not the ones the
> client requested.
> 
> Signed-off-by: Sylvain Garrigues <sylvain@sylvaingarrigues.com>
> ---
>  hw/misc/bcm2835_property.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index 530411f..0102144 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -21,6 +21,7 @@ static void
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>      int n;
>      uint32_t offset, length, color;
>      uint32_t xres, yres, xoffset, yoffset, bpp, pixo, alpha;
> +    uint32_t tmp_xres, tmp_yres, tmp_xoffset, tmp_yoffset, tmp_bpp,
> + tmp_pixo, tmp_alpha;
>      uint32_t *newxres = NULL, *newyres = NULL, *newxoffset = NULL,
>          *newyoffset = NULL, *newbpp = NULL, *newpixo = NULL, *newalpha =
> NULL;
> 
> @@ -139,7 +140,10 @@ static void
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
> 
>          case 0x00040001: /* Allocate buffer */
>              stl_le_phys(&s->dma_as, value + 12, s->fbdev->base);
> -            stl_le_phys(&s->dma_as, value + 16, s->fbdev->size);
> +            tmp_xres = newxres != NULL ? *newxres : s->fbdev->xres;
> +            tmp_yres = newyres != NULL ? *newyres : s->fbdev->yres;
> +            tmp_bpp = newbpp != NULL ? *newbpp : s->fbdev->bpp;
> +            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres *
> + (tmp_bpp >> 3));

Personal style nit: I prefer * 8 rather than >> 3, because it's more immediately obvious what you're computing, a trivial optimisation for the compiler, and in this specific example wouldn't need the brackets to ensure the correct operator precedence.

But in any case,
Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>

Thanks,
Andrew

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

Le 22 avr. 2016 à 13:22, Andrew Baumann <Andrew.Baumann@microsoft.com> a écrit :
>> +            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres *
>> + (tmp_bpp >> 3));
> 
> Personal style nit: I prefer * 8 rather than >> 3, because it's more immediately obvious what you're computing, a trivial optimisation for the compiler, and in this specific example wouldn't need the brackets to ensure the correct operator precedence.
> 
> But in any case,
> Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>


Thanks! It is actually / 8, but I used >> 3 to remain consistent with what I read in hw/display/bcm2835_fb.c. Feel free to adapt.

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Andrew Baumann]

> From: Sylvain Garrigues [mailto:sylvain@sylvaingarrigues.com]
> Sent: Friday, 22 April 2016 13:27
> 
> Le 22 avr. 2016 à 13:22, Andrew Baumann
> <Andrew.Baumann@microsoft.com> a écrit :
> >> +            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres
> >> + * (tmp_bpp >> 3));
> >
> > Personal style nit: I prefer * 8 rather than >> 3, because it's more immediately
> obvious what you're computing, a trivial optimisation for the compiler, and in
> this specific example wouldn't need the brackets to ensure the correct operator
> precedence.
> >
> > But in any case,
> > Reviewed-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> 
> 
> Thanks! It is actually / 8, but I used >> 3 to remain consistent with what I read
> in hw/display/bcm2835_fb.c. Feel free to adapt.

Duh, yes sorry it's obviously /8. The _fb code was probably like that in the original version.

But I won't be merging this patch (hopefully Peter can do that?), so it's probably best if you make the tweak and resend with my Reviewed-by.

Andrew

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Sylvain Garrigues]

Le 22 avr. 2016 à 13:30, Andrew Baumann <Andrew.Baumann@microsoft.com> a écrit :
> But I won't be merging this patch (hopefully Peter can do that?), so it's probably best if you make the tweak and resend with my Reviewed-by.

Made the tweak and resent the patch with your reviewed-by.

Have a good day.

Re: [Qemu-devel] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Paolo Bonzini]

On 22/04/2016 13:30, Andrew Baumann wrote:
>> From: Sylvain Garrigues [mailto:sylvain@sylvaingarrigues.com]
>> Sent: Friday, 22 April 2016 13:27
>>
>> Le 22 avr. 2016 à 13:22, Andrew Baumann
>> <Andrew.Baumann@microsoft.com> a écrit :
>>>> +            stl_le_phys(&s->dma_as, value + 16, tmp_xres * tmp_yres
>>>> + * (tmp_bpp >> 3));
>>>
>>> Personal style nit: I prefer * 8 rather than >> 3, because it's more immediately
>> obvious what you're computing, a trivial optimisation for the compiler

Note that it's only trivial for unsigned dividend.  A signed dividend
has >> rounding towards -infinity and / rounding towards zero.

Paolo