Re: [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts

[Igor Mammedov]

On Mon, 26 Oct 2020 12:39:24 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:

> According to PCIe spec 5.0 Type 1 header space Base Address Registers
> are defined by 7.5.1.2.1 Base Address Registers (same as Type 0). The
> _CRS region should allow for the same range (up to 64b). Prior to this
> change, any host bridge utilizing more than 32b for the BAR would have
> the address truncated and likely lead to conflicts when the operating
> systems reads the _CRS object.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

Looks good to me, so

Reviewed-by: Igor Mammedov <imammedo@redhat.com>


CCing,
Michael to have a send pair of eyes on it 

but I wonder how/why ivshm (which might have quite large BAR) works.

PS:
please use git's --cover-letter option to create multi-patch series,
in the future

> 
> ---
> I don't think this effects any code currently in QEMU. You'd need to
> have a host bridge which has a BAR, and that BAR wants to be > 32b. I've
> hit this because I have a modified PXB device that does advertise a 64b
> BAR. Also, you'd need a platform that cares about ACPI, which, many do
> not.
> ---
>  hw/i386/acpi-build.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index df13abecf4..75604bdc74 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -789,8 +789,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet 
> *range_set)
>                  crs_range_insert(temp_range_set.io_ranges,
>                                   range_base, range_limit);
>              } else { /* "memory" */
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges, range_base,
> +                                     range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>          }
>