| Message ID | 20201026193924.985014-2-ben.widawsky@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] acpi/crs: Prevent bad ranges for host bridges | expand |
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); > + } > } > } >
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); > > + } > > } > > } > > > >
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. [...]
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); + } } }
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(-)