Re: [Qemu-devel] [PATCH 2/4] i386/acpi-build: fix PXB workarounds for unsupported BIOSes

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/06/2015 02:46 AM, Laszlo Ersek wrote:
> The patch
>
>    apci: fix PXB behaviour if used with unsupported BIOS
>
> uses the following condition to see if a "PXB mem/IO chunk" has *not* been
> configured by the BIOS:
>
>    (!range_base || range_base > range_limit)
>
> When this condition evaluates to true, said patch *omits* the
> corresponding entry from the _CRS.
>
> Later on the patch checks for the opposite condition (with the intent of
> *adding* entries to the _CRS if the "PXB mem/IO chunks" *have* been
> configured). Unfortunately, the condition was negated incorrectly: only
> the first ! operator was removed, which led to the nonsensical expression
>
>    (range_base || range_base > range_limit)
>
> leading to bogus entries in the _CRS, and causing BSOD in Windows Server
> 2012 R2 when it runs on OVMF.
Thanks for catching this!

>
> The correct negative of the condition seen at the top is
>
>    (range_base && range_base <= range_limit)
>
> Fix the expressions.
>
> Cc: Marcel Apfelbaum <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   hw/i386/acpi-build.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 52c2591..b71e942 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -833,7 +833,7 @@ static Aml *build_crs(PCIHostState *host,
>                * Work-around for old bioses
>                * that do not support multiple root buses
>                */
> -            if (range_base || range_base > range_limit) {
> +            if (range_base && range_base <= range_limit) {
>                   aml_append(crs,
>                              aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
>                                          AML_POS_DECODE, AML_ENTIRE_RANGE,
> @@ -854,7 +854,7 @@ static Aml *build_crs(PCIHostState *host,
>                * Work-around for old bioses
>                * that do not support multiple root buses
>                */
> -            if (range_base || range_base > range_limit) {
> +            if (range_base && range_base <= range_limit) {
>                   aml_append(crs,
>                              aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>                                               AML_MAX_FIXED, AML_NON_CACHEABLE,
> @@ -865,7 +865,7 @@ static Aml *build_crs(PCIHostState *host,
>                                               0,
>                                               range_limit - range_base + 1));
>                   crs_range_insert(mem_ranges, range_base, range_limit);
> -          }
> +            }
>
>               range_base =
>                   pci_bridge_get_base(dev, PCI_BASE_ADDRESS_MEM_PREFETCH);
> @@ -876,7 +876,7 @@ static Aml *build_crs(PCIHostState *host,
>                * Work-around for old bioses
>                * that do not support multiple root buses
>                */
> -            if (range_base || range_base > range_limit) {
> +            if (range_base && range_base <= range_limit) {
>                   aml_append(crs,
>                              aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>                                               AML_MAX_FIXED, AML_NON_CACHEABLE,
>

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel