[PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges

[PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges

[Ben Widawsky]

Prevent _CRS resources being quietly chopped off and instead throw an
assertion. _CRS is used by host bridges to declare regions of io and/or
memory that they consume. On some (all?) platforms the host bridge
doesn't have PCI header space and so they need some way to convey the
information.

Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

---

1. I'm not aware of this being a real issue on platforms today as I think
   many platforms that use ACPI and actually have regions, constrain to
   32b to be complaint with legacy.
2. Since host bridges aren't usually hot plugged, it can't be invoked by
   a user, so assert() seems like the right way to handle this.
---
 hw/i386/acpi-build.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index eee7b57c37..df13abecf4 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -869,6 +869,8 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
     crs_range_merge(temp_range_set.mem_ranges);
     for (i = 0; i < temp_range_set.mem_ranges->len; i++) {
         entry = g_ptr_array_index(temp_range_set.mem_ranges, i);
+        assert(entry->limit <= UINT32_MAX &&
+               (entry->limit - entry->base + 1) <= UINT32_MAX);
         aml_append(crs,
                    aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
                                     AML_MAX_FIXED, AML_NON_CACHEABLE,
-- 
2.29.1

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

[Ben Widawsky]

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>

---
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);
+                }
             }
         }
 
-- 
2.29.1

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);
> +                }
>              }
>          }
>  

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

[Ben Widawsky]

On 20-10-27 15:36:12, Igor Mammedov wrote:
> 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.

I think this will only hit things that subclass TYPE_PCI_HOST_BRIDGE. AFAICT,
ivshm is a regular TYPE_PCI_DEVICE. Is there a _CRS created for ivshm?

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

Will do. I wasn't sure what the cutoff was, but the wiki is pretty clear that
"multiple" is the cutoff and it's important to CI.
> 
> > 
> > ---
> > 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);
> > +                }
> >              }
> >          }
> >  
> 
> 

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

[Igor Mammedov]

On Tue, 27 Oct 2020 08:45:05 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On 20-10-27 15:36:12, Igor Mammedov wrote:
> > 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.  
> 
> I think this will only hit things that subclass TYPE_PCI_HOST_BRIDGE. AFAICT,
> ivshm is a regular TYPE_PCI_DEVICE. Is there a _CRS created for ivshm?

no, but device uses _CRS provided by bus, so I'd expect it would fail
on guest side if its BAR is bigger than window provided by host bridge.

[...]

Re: [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges

[Igor Mammedov]

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

> Prevent _CRS resources being quietly chopped off and instead throw an
> assertion. _CRS is used by host bridges to declare regions of io and/or
> memory that they consume. On some (all?) platforms the host bridge
> doesn't have PCI header space and so they need some way to convey the
> information.
> 
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>

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

> ---
> 
> 1. I'm not aware of this being a real issue on platforms today as I think
>    many platforms that use ACPI and actually have regions, constrain to
>    32b to be complaint with legacy.
> 2. Since host bridges aren't usually hot plugged, it can't be invoked by
>    a user, so assert() seems like the right way to handle this.
> ---
>  hw/i386/acpi-build.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index eee7b57c37..df13abecf4 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -869,6 +869,8 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>      crs_range_merge(temp_range_set.mem_ranges);
>      for (i = 0; i < temp_range_set.mem_ranges->len; i++) {
>          entry = g_ptr_array_index(temp_range_set.mem_ranges, i);
> +        assert(entry->limit <= UINT32_MAX &&
> +               (entry->limit - entry->base + 1) <= UINT32_MAX);
>          aml_append(crs,
>                     aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>                                      AML_MAX_FIXED, AML_NON_CACHEABLE,