[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 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] [Qemu-arm] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Stephen Warren]

On 04/21/2016 08:07 AM, Sylvain Garrigues wrote:
> 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.

It would indeed be nice if qemu for the Pi implemented the exact same 
bootloader setup as real HW does. That is, loading boot images from the 
FAT partition on the SD card, parsing config.txt, etc.

Ideally as was mentioned earlier this would be done by simply executing 
the existing bootloader under emulation, rather than building all that 
code into qemu. However, in the Pi case, the bootloader runs on the 
VideoCore (a separate non-ARM CPU), so isn't (and likely won't be since 
IIUC it isn't fully documented) emulated by qemu. by the time the ARM 
CPU runs, everything (kernel, DTB/ATAGS, ARM boot stub, ...) is already 
loaded into RAM, the display is already probed over HDMI and the 
controller scanning out a dummy image, etc.

So I think if that were to be supported, it'd have to be coded into 
qemu. Is that something that could happen, or would such patches not fit 
qemu's model well?

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

[Peter Crosthwaite]

On Thu, Apr 21, 2016 at 9:06 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/21/2016 08:07 AM, Sylvain Garrigues wrote:
>>
>> 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.
>
>
> It would indeed be nice if qemu for the Pi implemented the exact same
> bootloader setup as real HW does. That is, loading boot images from the FAT
> partition on the SD card, parsing config.txt, etc.
>
> Ideally as was mentioned earlier this would be done by simply executing the
> existing bootloader under emulation, rather than building all that code into
> qemu. However, in the Pi case, the bootloader runs on the VideoCore (a
> separate non-ARM CPU), so isn't (and likely won't be since IIUC it isn't
> fully documented) emulated by qemu. by the time the ARM CPU runs, everything
> (kernel, DTB/ATAGS, ARM boot stub, ...) is already loaded into RAM, the
> display is already probed over HDMI and the controller scanning out a dummy
> image, etc.
>
> So I think if that were to be supported, it'd have to be coded into qemu. Is
> that something that could happen, or would such patches not fit qemu's model
> well?

I made half a start on this project but had to shelve it.

The hard part is the FAT filesystem. The basic approach I started on
was to link in the relevant bits of the GNU mtools so QEMU can poke
around in a FAT filesystem hosted on a block device. Then just mount
the SD card and emulate the boot logic of the VideoCore bootloader.
This amount of new code should actually be pretty small, as the FS
driver is handled by mtools and the SDHCI can be shorted by just
having this alternate bootloader go direct to the block device (fish
the blockdev out from the SD card).

Regards,
Peter

>

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

[Gerd Hoffmann]

  Hi,

> > Ideally as was mentioned earlier this would be done by simply executing the
> > existing bootloader under emulation, rather than building all that code into
> > qemu. However, in the Pi case, the bootloader runs on the VideoCore (a
> > separate non-ARM CPU), so isn't (and likely won't be since IIUC it isn't
> > fully documented) emulated by qemu. by the time the ARM CPU runs, everything
> > (kernel, DTB/ATAGS, ARM boot stub, ...) is already loaded into RAM, the
> > display is already probed over HDMI and the controller scanning out a dummy
> > image, etc.
> >
> > So I think if that were to be supported, it'd have to be coded into qemu. Is
> > that something that could happen, or would such patches not fit qemu's model
> > well?
> 
> I made half a start on this project but had to shelve it.
> 
> The hard part is the FAT filesystem. The basic approach I started on
> was to link in the relevant bits of the GNU mtools so QEMU can poke
> around in a FAT filesystem hosted on a block device. Then just mount
> the SD card and emulate the boot logic of the VideoCore bootloader.
> This amount of new code should actually be pretty small, as the FS
> driver is handled by mtools and the SDHCI can be shorted by just
> having this alternate bootloader go direct to the block device (fish
> the blockdev out from the SD card).

Alternatively we can just go for a later boot loader stage, i.e. put a
u-boot build for rpi2 to pc-bios/ (we already have one for ppc there)
and run that by default.

Our sdcard emulation seems to have problems though:

   U-Boot 2016.05-rc2 (Apr 22 2016 - 09:11:45 +0200)

   DRAM:  960 MiB
   RPI 2 Model B (0xa21041)
   MMC:   <uboot hangs here>

Recent linux kernels have trouble talking to the mmc too.

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] [Qemu-arm] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Andrew Baumann]

Hi all,

> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
> Sent: Friday, 22 April 2016 09:18
> 
> On Thu, Apr 21, 2016 at 9:06 AM, Stephen Warren
> <swarren@wwwdotorg.org> wrote:
> > On 04/21/2016 08:07 AM, Sylvain Garrigues wrote:
> >>
> >> 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.
> >
> >
> > It would indeed be nice if qemu for the Pi implemented the exact same
> > bootloader setup as real HW does. That is, loading boot images from
> > the FAT partition on the SD card, parsing config.txt, etc.
> >
> > Ideally as was mentioned earlier this would be done by simply
> > executing the existing bootloader under emulation, rather than
> > building all that code into qemu. However, in the Pi case, the
> > bootloader runs on the VideoCore (a separate non-ARM CPU), so isn't
> > (and likely won't be since IIUC it isn't fully documented) emulated by
> > qemu. by the time the ARM CPU runs, everything (kernel, DTB/ATAGS, ARM
> > boot stub, ...) is already loaded into RAM, the display is already
> > probed over HDMI and the controller scanning out a dummy image, etc.
> >
> > So I think if that were to be supported, it'd have to be coded into
> > qemu. Is that something that could happen, or would such patches not
> > fit qemu's model well?
> 
> I made half a start on this project but had to shelve it.
> 
> The hard part is the FAT filesystem. The basic approach I started on was to link
> in the relevant bits of the GNU mtools so QEMU can poke around in a FAT
> filesystem hosted on a block device. Then just mount the SD card and emulate
> the boot logic of the VideoCore bootloader.
> This amount of new code should actually be pretty small, as the FS driver is
> handled by mtools and the SDHCI can be shorted by just having this alternate
> bootloader go direct to the block device (fish the blockdev out from the SD
> card).

You got further on this than I did. I considered a couple of options, of varying complexity/compatibility:

 0. The status quo: we support -kernel (for Linux images) and -bios (e.g. Windows), but otherwise all the options that can be set in config.txt are treated as the defaults. For example, -bios images are always loaded at 0x8000. Additionally, framebuffer parameters can be set directly from the command line (e.g. Windows wants -global bcm2835-fb.pixo=0).
 1. More pi-specific parameters in the line of the framebuffer parameters above, to control things like the image load address, and maybe to enable trustzone boot akin to config.txt's kernel-old=1.
 2. Code to parse config.txt and set the parameters above.
 3. Code to emulate the bootloader entirely, pulling config.txt and all the relevant boot bits out of an SD image (this is what Peter describes above).
 4. An ARM-based reimplementation of the bootloader, running under emulation.

I discarded even considering option 3 because I assumed you wouldn't want a FAT implementation linked into qemu. And I'm not inclined to add ini-parsing code, so my gut feeling was to go a little further on option 1 and add parameters to the raspi machines (I assume this is possible?) for basic things like the load address, and rely on users to translate config.txt settings into command-line parameters.

Andrew

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

[Andrew Baumann]

> From: Gerd Hoffmann [mailto:kraxel@redhat.com]
> Sent: Friday, 22 April 2016 09:46
>
> Alternatively we can just go for a later boot loader stage, i.e. put a u-boot build
> for rpi2 to pc-bios/ (we already have one for ppc there) and run that by default.
> 
> Our sdcard emulation seems to have problems though:
> 
>    U-Boot 2016.05-rc2 (Apr 22 2016 - 09:11:45 +0200)
> 
>    DRAM:  960 MiB
>    RPI 2 Model B (0xa21041)
>    MMC:   <uboot hangs here>
> 
> Recent linux kernels have trouble talking to the mmc too.

I know :( I haven't looked at the more recent kernels, but it could be the same issue I spent a while debugging fruitlessly on raspi1 -- it looked like Linux was issuing a large MMC read for N blocks, but only programming the DMA controller for fewer (<N*blocksize). The DMA reads completed quickly but eventually the kernel timed out, logged an error and reissued the whole operation as single-block PIO reads.

If you get a chance to diagnose this any further, I'd be curious to know what you find. If the same problem repros with u-boot, it's almost certainly simpler to debug there.

Thanks,
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] [Qemu-arm] [PATCH] bcm2835_property: use cached values when querying framebuffer

[Stephen Warren]

On 04/22/2016 01:46 AM, Gerd Hoffmann wrote:
>    Hi,
>
>>> Ideally as was mentioned earlier this would be done by simply executing the
>>> existing bootloader under emulation, rather than building all that code into
>>> qemu. However, in the Pi case, the bootloader runs on the VideoCore (a
>>> separate non-ARM CPU), so isn't (and likely won't be since IIUC it isn't
>>> fully documented) emulated by qemu. by the time the ARM CPU runs, everything
>>> (kernel, DTB/ATAGS, ARM boot stub, ...) is already loaded into RAM, the
>>> display is already probed over HDMI and the controller scanning out a dummy
>>> image, etc.
>>>
>>> So I think if that were to be supported, it'd have to be coded into qemu. Is
>>> that something that could happen, or would such patches not fit qemu's model
>>> well?
>>
>> I made half a start on this project but had to shelve it.
>>
>> The hard part is the FAT filesystem. The basic approach I started on
>> was to link in the relevant bits of the GNU mtools so QEMU can poke
>> around in a FAT filesystem hosted on a block device. Then just mount
>> the SD card and emulate the boot logic of the VideoCore bootloader.
>> This amount of new code should actually be pretty small, as the FS
>> driver is handled by mtools and the SDHCI can be shorted by just
>> having this alternate bootloader go direct to the block device (fish
>> the blockdev out from the SD card).
>
> Alternatively we can just go for a later boot loader stage, i.e. put a
> u-boot build for rpi2 to pc-bios/ (we already have one for ppc there)
> and run that by default.

The disadvantage here is that people might expect all their options in 
config.txt (e.g. kernel command-line, load address, DTB overlays, GPU 
RAM size, etc.) to be honored on any Pi platform. If we jump straight 
into U-Boot, they won't be, at least not in the typical Pi way.

> Our sdcard emulation seems to have problems though:
>
>     U-Boot 2016.05-rc2 (Apr 22 2016 - 09:11:45 +0200)
>
>     DRAM:  960 MiB
>     RPI 2 Model B (0xa21041)
>     MMC:   <uboot hangs here>

This particular issue is because the bcm2835 timer isn't emulated, and 
the U-Boot MMC driver call udelay or similar, which is implemented by 
waiting for that timer to tick. There's a patch in Andrew's github tree 
that implements the timer, and with that U-Boot at least gets to its 
shell prompt, or did the last time I tested it.

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

[Peter Crosthwaite]

On Fri, Apr 22, 2016 at 12:46 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> > Ideally as was mentioned earlier this would be done by simply executing the
>> > existing bootloader under emulation, rather than building all that code into
>> > qemu. However, in the Pi case, the bootloader runs on the VideoCore (a
>> > separate non-ARM CPU), so isn't (and likely won't be since IIUC it isn't
>> > fully documented) emulated by qemu. by the time the ARM CPU runs, everything
>> > (kernel, DTB/ATAGS, ARM boot stub, ...) is already loaded into RAM, the
>> > display is already probed over HDMI and the controller scanning out a dummy
>> > image, etc.
>> >
>> > So I think if that were to be supported, it'd have to be coded into qemu. Is
>> > that something that could happen, or would such patches not fit qemu's model
>> > well?
>>
>> I made half a start on this project but had to shelve it.
>>
>> The hard part is the FAT filesystem. The basic approach I started on
>> was to link in the relevant bits of the GNU mtools so QEMU can poke
>> around in a FAT filesystem hosted on a block device. Then just mount
>> the SD card and emulate the boot logic of the VideoCore bootloader.
>> This amount of new code should actually be pretty small, as the FS
>> driver is handled by mtools and the SDHCI can be shorted by just
>> having this alternate bootloader go direct to the block device (fish
>> the blockdev out from the SD card).
>
> Alternatively we can just go for a later boot loader stage, i.e. put a
> u-boot build for rpi2 to pc-bios/ (we already have one for ppc there)
> and run that by default.
>

Rather than a later boot stage, could we set this up to be identical
in functionality to the early boot stage? This enables baremetal devs
who want to debug from the real bootrom handoff (handoff from
VideoCore).

Doing it over FAT+bootloader code does however have the advantage of
not having to go through emulated SD card and TCG.

Regards,
Peter

> Our sdcard emulation seems to have problems though:
>
>    U-Boot 2016.05-rc2 (Apr 22 2016 - 09:11:45 +0200)
>
>    DRAM:  960 MiB
>    RPI 2 Model B (0xa21041)
>    MMC:   <uboot hangs here>
>
> Recent linux kernels have trouble talking to the mmc too.

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

[Peter Crosthwaite]

On Fri, Apr 22, 2016 at 4:44 AM, Andrew Baumann
<Andrew.Baumann@microsoft.com> wrote:
> Hi all,
>
>> From: Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com]
>> Sent: Friday, 22 April 2016 09:18
>>
>> On Thu, Apr 21, 2016 at 9:06 AM, Stephen Warren
>> <swarren@wwwdotorg.org> wrote:
>> > On 04/21/2016 08:07 AM, Sylvain Garrigues wrote:
>> >>
>> >> 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.
>> >
>> >
>> > It would indeed be nice if qemu for the Pi implemented the exact same
>> > bootloader setup as real HW does. That is, loading boot images from
>> > the FAT partition on the SD card, parsing config.txt, etc.
>> >
>> > Ideally as was mentioned earlier this would be done by simply
>> > executing the existing bootloader under emulation, rather than
>> > building all that code into qemu. However, in the Pi case, the
>> > bootloader runs on the VideoCore (a separate non-ARM CPU), so isn't
>> > (and likely won't be since IIUC it isn't fully documented) emulated by
>> > qemu. by the time the ARM CPU runs, everything (kernel, DTB/ATAGS, ARM
>> > boot stub, ...) is already loaded into RAM, the display is already
>> > probed over HDMI and the controller scanning out a dummy image, etc.
>> >
>> > So I think if that were to be supported, it'd have to be coded into
>> > qemu. Is that something that could happen, or would such patches not
>> > fit qemu's model well?
>>
>> I made half a start on this project but had to shelve it.
>>
>> The hard part is the FAT filesystem. The basic approach I started on was to link
>> in the relevant bits of the GNU mtools so QEMU can poke around in a FAT
>> filesystem hosted on a block device. Then just mount the SD card and emulate
>> the boot logic of the VideoCore bootloader.
>> This amount of new code should actually be pretty small, as the FS driver is
>> handled by mtools and the SDHCI can be shorted by just having this alternate
>> bootloader go direct to the block device (fish the blockdev out from the SD
>> card).
>
> You got further on this than I did. I considered a couple of options, of varying complexity/compatibility:
>
>  0. The status quo: we support -kernel (for Linux images) and -bios (e.g. Windows), but otherwise all the options that can be set in config.txt are treated as the defaults. For example, -bios images are always loaded at 0x8000. Additionally, framebuffer parameters can be set directly from the command line (e.g. Windows wants -global bcm2835-fb.pixo=0).
>  1. More pi-specific parameters in the line of the framebuffer parameters above, to control things like the image load address, and maybe to enable trustzone boot akin to config.txt's kernel-old=1.
>  2. Code to parse config.txt and set the parameters above.
>  3. Code to emulate the bootloader entirely, pulling config.txt and all the relevant boot bits out of an SD image (this is what Peter describes above).
>  4. An ARM-based reimplementation of the bootloader, running under emulation.

Setting up U-boot to be #4 could work.

>
> I discarded even considering option 3 because I assumed you wouldn't want a FAT implementation linked into qemu.

rPI is not alone here though. Xilinx Zynq does exactly the same thing
with boot code pulling things out of SD card + FAT so you have at
least two precedents justifying the feature.

Regards,
Peter

> And I'm not inclined to add ini-parsing code, so my gut feeling was to go a little further on option 1 and add parameters to the raspi machines (I assume this is possible?) for basic things like the load address, and rely on users to translate config.txt settings into command-line parameters.
>
> Andrew

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

[Sylvain Garrigues]

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

Le 22 avr. 2016 à 13:44, Andrew Baumann <Andrew.Baumann@microsoft.com> a écrit :
> You got further on this than I did. I considered a couple of options, of varying complexity/compatibility:
> 
> 0. The status quo: we support -kernel (for Linux images) and -bios (e.g. Windows), but otherwise all the options that can be set in config.txt are treated as the defaults. For example, -bios images are always loaded at 0x8000. Additionally, framebuffer parameters can be set directly from the command line (e.g. Windows wants -global bcm2835-fb.pixo=0).
> 1. More pi-specific parameters in the line of the framebuffer parameters above, to control things like the image load address, and maybe to enable trustzone boot akin to config.txt's kernel-old=1.
> 2. Code to parse config.txt and set the parameters above.
> 3. Code to emulate the bootloader entirely, pulling config.txt and all the relevant boot bits out of an SD image (this is what Peter describes above).
> 4. An ARM-based reimplementation of the bootloader, running under emulation.


Having the ARM bootloader (called armstub in raspberry pi developers words), the config.txt file, and the « kernel » (whatever it is, ELF or raw binary) on a FAT portion of an SD card is convenient for real hardware: on a lambda user perspective, you just use your lambda computer to burn an image on a SD card (one you don’t use anymore with your camera), then insert it in your shiny new raspberry pi hardware.

QEMU in my opinion does not target the regular lambda user, and I wouldn’t go as far as mimicking the FAT / SD emulation boot process: my personal use case for qemu as a developer is « kernel » development / debugging and my workflow sums up to a simple loop: 

# vi some_kernel_source_code.c && make && qemu -kernel the_new_kernel_image
Do that 100 times a day.

Having to place my kernel in a FAT partition of a mounted disk image in my host OS would just complexity my process. I don’t expect my emulator to be 100% similar to the real hardware (it will never in the case of the Raspberry Pi, as long as the GPU firmware source code is closed). I just want to use my emulator to get things done fast (on my host computer rather than real hardware) and I want to emulate CPU and code (interrupts, etc) as closely as possible, without too much effort.

Having said that, I would give the qemu user who wants raspberry pi emulation these two functionalities that the raspberry pi firmware provides:
1/ ability to use a custom bootloader before launching the kernel: option armstub=somebootloader.bin in config.txt (be it u-boot or a custom stub which leaves HYP mode)
2/ ability to load your « kernel » at a specific address: option kernel_address in config.txt

So future -kernel-address and -bootloader qemu command line parameters (either specific to Raspberry pi or global) is my dream. That way I can play fast with whatever bootloader I want, play with HYP mode, and I can use whatever loading address I want. 

By the way, for the Raspberry Pi, the address is 0x8000 compared to the default 0x10000 for Arm in qemu, and being able to set it would enable to boot (without patching QEMU’s code) NetBSD images (compiled to work at 0x8000) and FreeBSD kernel without u-boot (requires the loading address to be a multiple of 2MB). Seems to me like basic functionality. I heard your argument Peter mentioning the linux boot protocol doesn’t mandate a loading address, and I find it is for this specific reason that which should let the user choose it. I also heard the « use an ELF » argument but some non-Linux kernels have by default a virtual (and not physical) address in their header, so doesn’t work straight out of the box.

Just my 2 cents :-) I however want to thank you all for the quality of the ARM emulation and more specifically the Raspberry Pi one, it opens many possibilities and is charm to work with.

Have a very good week-end all,
Sylvain.

[-- Attachment #2: Type: text/html, Size: 9268 bytes --]

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

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

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

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

[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