[Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Stefan Weil]

Hi,

since commit 21f5826a04d38e19488f917e1eef22751490c769
"seabios: update to 1.8.0 release" there is a misaligned i/o access
caused by the PC BIOS.

QEMU's PC emulation (qemu-system-i386, qemu-system-x86_64)
with enabled trace backend reports the misaligned i/o access
when running the BIOS code:

Misaligned i/o to address 0x0000ffff with size 4 for memory region io

("to address 0x0000ffff" was added by my debug code)

This issue was reported by users of QEMU for Windows,
but it is not Windows specific.

Regards
Stefan

Re: [Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Stefan Weil]

Am 26.07.2015 um 21:32 schrieb Stefan Weil:
> Hi,
>
> since commit 21f5826a04d38e19488f917e1eef22751490c769
> "seabios: update to 1.8.0 release" there is a misaligned i/o access
> caused by the PC BIOS.
>
> QEMU's PC emulation (qemu-system-i386, qemu-system-x86_64)
> with enabled trace backend reports the misaligned i/o access
> when running the BIOS code:
>
> Misaligned i/o to address 0x0000ffff with size 4 for memory region io
>
> ("to address 0x0000ffff" was added by my debug code)
>
> This issue was reported by users of QEMU for Windows,
> but it is not Windows specific.
>
> Regards
> Stefan


Sorry, I did not notice that the code which reports misaligned access is
not part of the official QEMU code.

Here is the patch for memory.c which adds it:

@@ -1073,6 +1119,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
      int access_size, i;

      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
+        fprintf(stderr, "Misaligned i/o to address %08" HWADDR_PRIx
+                        " with size %u for memory region %s\n",
+                addr, size, mr->name);
          return false;
      }

Is the misaligned i/o access done by the BIOS code a feature or a bug?

Regards
Stefan

Re: [Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Stefan Weil]

Am 27.07.2015 um 07:49 schrieb Stefan Weil:
> Am 26.07.2015 um 21:32 schrieb Stefan Weil:
>> Hi,
>>
>> since commit 21f5826a04d38e19488f917e1eef22751490c769
>> "seabios: update to 1.8.0 release" there is a misaligned i/o access
>> caused by the PC BIOS.
>>
>> QEMU's PC emulation (qemu-system-i386, qemu-system-x86_64)
>> with enabled trace backend reports the misaligned i/o access
>> when running the BIOS code:
>>
>> Misaligned i/o to address 0x0000ffff with size 4 for memory region io
>>
>> ("to address 0x0000ffff" was added by my debug code)
>>
>> This issue was reported by users of QEMU for Windows,
>> but it is not Windows specific.
>>
>> Regards
>> Stefan
>
>
> Sorry, I did not notice that the code which reports misaligned access is
> not part of the official QEMU code.
>
> Here is the patch for memory.c which adds it:
>
> @@ -1073,6 +1119,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
>      int access_size, i;
>
>      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> +        fprintf(stderr, "Misaligned i/o to address %08" HWADDR_PRIx
> +                        " with size %u for memory region %s\n",
> +                addr, size, mr->name);
>          return false;
>      }
>
> Is the misaligned i/o access done by the BIOS code a feature or a bug?
>
> Regards
> Stefan
>

The change (regression?) was caused by this commit (which obviously
did not intend to change generated code):

40d03128531e06b35a3836f9346790003692540e is the first bad commit
commit 40d03128531e06b35a3836f9346790003692540e
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu May 15 13:22:26 2014 +0200

     piix: add and use dev-piix.h

     Move all definitions for PIIX registers to a single header file,
     like there is one already for Q35, and make the naming more
     consistent.

     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

:040000 040000 72d3a611f87106916d58e30fa1d615659fa711f6 
5bfd5601430bc5263ac0a6f8a15a131f3bc8678e M    src

Re: [Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Stefan Weil]

Am 27.07.2015 um 08:24 schrieb Stefan Weil:
> Am 27.07.2015 um 07:49 schrieb Stefan Weil:
>> Am 26.07.2015 um 21:32 schrieb Stefan Weil:
>>> Hi,
>>>
>>> since commit 21f5826a04d38e19488f917e1eef22751490c769
>>> "seabios: update to 1.8.0 release" there is a misaligned i/o access
>>> caused by the PC BIOS.
>>>
>>> QEMU's PC emulation (qemu-system-i386, qemu-system-x86_64)
>>> with enabled trace backend reports the misaligned i/o access
>>> when running the BIOS code:
>>>
>>> Misaligned i/o to address 0x0000ffff with size 4 for memory region io
>>>
>>> ("to address 0x0000ffff" was added by my debug code)
>>>
>>> This issue was reported by users of QEMU for Windows,
>>> but it is not Windows specific.
>>>
>>> Regards
>>> Stefan
>>
>>
>> Sorry, I did not notice that the code which reports misaligned access is
>> not part of the official QEMU code.
>>
>> Here is the patch for memory.c which adds it:
>>
>> @@ -1073,6 +1119,9 @@ bool memory_region_access_valid(MemoryRegion *mr,
>>      int access_size, i;
>>
>>      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
>> +        fprintf(stderr, "Misaligned i/o to address %08" HWADDR_PRIx
>> +                        " with size %u for memory region %s\n",
>> +                addr, size, mr->name);
>>          return false;
>>      }
>>
>> Is the misaligned i/o access done by the BIOS code a feature or a bug?
>>
>> Regards
>> Stefan

Please ignore my last e-mail. This one is the commit reported
by git bisect for seabios:

457ba42878bd9f704e5a6c1c7bc7fcced686fe4e is the first bad commit
commit 457ba42878bd9f704e5a6c1c7bc7fcced686fe4e
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Thu May 15 13:22:28 2014 +0200

     smm: complete SMM setup

     SMI generation requires two bits to be set in PIIX4, one for APMC
     interrupts specifically and a general one.

     For Q35 it is the same, plus it is a good thing to lock SMIs after
     enabling them.

     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

:040000 040000 5bfd5601430bc5263ac0a6f8a15a131f3bc8678e 
5e0901b1c5ae4c79ed4c34c8bd27e7daf9006086 M    src

Re: [Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Paolo Bonzini]

On 27/07/2015 08:36, Stefan Weil wrote:
> commit 457ba42878bd9f704e5a6c1c7bc7fcced686fe4e
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Thu May 15 13:22:28 2014 +0200
> 
>     smm: complete SMM setup
> 
>     SMI generation requires two bits to be set in PIIX4, one for APMC
>     interrupts specifically and a general one.
> 
>     For Q35 it is the same, plus it is a good thing to lock SMIs after
>     enabling them.
> 
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> :040000 040000 5bfd5601430bc5263ac0a6f8a15a131f3bc8678e
> 5e0901b1c5ae4c79ed4c34c8bd27e7daf9006086 M    src
> 

Is this enough to fix it?

--- a/src/fw/smm.c
+++ b/src/fw/smm.c
@@ -184,7 +184,7 @@ static void piix4_apmc_smm_setup(int isabdf, int i440_bdf)
 
     /* enable SMI generation */
     value = inl(acpi_pm_base + PIIX_PMIO_GLBCTL);
-    outl(acpi_pm_base + PIIX_PMIO_GLBCTL, value | PIIX_PMIO_GLBCTL_SMI_EN);
+    outl(value | PIIX_PMIO_GLBCTL_SMI_EN, acpi_pm_base + PIIX_PMIO_GLBCTL);
 
     smm_relocate_and_restore();
 

Paolo

Re: [Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Stefan Weil]

Am 27.07.2015 um 10:46 schrieb Paolo Bonzini:
> On 27/07/2015 08:36, Stefan Weil wrote:
>> commit 457ba42878bd9f704e5a6c1c7bc7fcced686fe4e
>> Author: Paolo Bonzini <pbonzini@redhat.com>
>> Date:   Thu May 15 13:22:28 2014 +0200
>>
>>     smm: complete SMM setup
>>
>>     SMI generation requires two bits to be set in PIIX4, one for APMC
>>     interrupts specifically and a general one.
>>
>>     For Q35 it is the same, plus it is a good thing to lock SMIs after
>>     enabling them.
>>
>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> :040000 040000 5bfd5601430bc5263ac0a6f8a15a131f3bc8678e
>> 5e0901b1c5ae4c79ed4c34c8bd27e7daf9006086 M    src
> Is this enough to fix it?
>
> --- a/src/fw/smm.c
> +++ b/src/fw/smm.c
> @@ -184,7 +184,7 @@ static void piix4_apmc_smm_setup(int isabdf, int i440_bdf)
>  
>      /* enable SMI generation */
>      value = inl(acpi_pm_base + PIIX_PMIO_GLBCTL);
> -    outl(acpi_pm_base + PIIX_PMIO_GLBCTL, value | PIIX_PMIO_GLBCTL_SMI_EN);
> +    outl(value | PIIX_PMIO_GLBCTL_SMI_EN, acpi_pm_base + PIIX_PMIO_GLBCTL);
>  
>      smm_relocate_and_restore();
>  
>
> Paolo


Yes, and the new code also looks much better for an outl function call. :-)
You may add

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Stefan

Re: [Qemu-devel] BIOS regression since v2.3.0 (misaligned longword i/o to address 0xffff)

[Stefan Weil]

Am 27.07.2015 um 11:32 schrieb Stefan Weil:
> Am 27.07.2015 um 10:46 schrieb Paolo Bonzini:
>> On 27/07/2015 08:36, Stefan Weil wrote:
>>> commit 457ba42878bd9f704e5a6c1c7bc7fcced686fe4e
>>> Author: Paolo Bonzini <pbonzini@redhat.com>
>>> Date:   Thu May 15 13:22:28 2014 +0200
>>>
>>>     smm: complete SMM setup
>>>
>>>     SMI generation requires two bits to be set in PIIX4, one for APMC
>>>     interrupts specifically and a general one.
>>>
>>>     For Q35 it is the same, plus it is a good thing to lock SMIs after
>>>     enabling them.
>>>
>>>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>> :040000 040000 5bfd5601430bc5263ac0a6f8a15a131f3bc8678e
>>> 5e0901b1c5ae4c79ed4c34c8bd27e7daf9006086 M    src
>> Is this enough to fix it?
>>
>> --- a/src/fw/smm.c
>> +++ b/src/fw/smm.c
>> @@ -184,7 +184,7 @@ static void piix4_apmc_smm_setup(int isabdf, int i440_bdf)
>>  
>>      /* enable SMI generation */
>>      value = inl(acpi_pm_base + PIIX_PMIO_GLBCTL);
>> -    outl(acpi_pm_base + PIIX_PMIO_GLBCTL, value | PIIX_PMIO_GLBCTL_SMI_EN);
>> +    outl(value | PIIX_PMIO_GLBCTL_SMI_EN, acpi_pm_base + PIIX_PMIO_GLBCTL);
>>  
>>      smm_relocate_and_restore();
>>  
>>
>> Paolo
> 
> 
> Yes, and the new code also looks much better for an outl function call. :-)
> You may add
> 
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> 
> Stefan


When I look at src/hw/megasas.c, I see three more outl function calls
with wrong argument order. CC'ing Hannes therefore.

Stefan