* [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges
@ 2020-10-26 19:39 Ben Widawsky
2020-10-26 19:39 ` [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts Ben Widawsky
2020-10-29 13:59 ` [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges Igor Mammedov
0 siblings, 2 replies; 6+ messages in thread
From: Ben Widawsky @ 2020-10-26 19:39 UTC (permalink / raw)
To: qemu-devel
Cc: Ben Widawsky, Eduardo Habkost, Michael S. Tsirkin, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts
2020-10-26 19:39 [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges Ben Widawsky
@ 2020-10-26 19:39 ` Ben Widawsky
2020-10-27 14:36 ` Igor Mammedov
2020-10-29 13:59 ` [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges Igor Mammedov
1 sibling, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2020-10-26 19:39 UTC (permalink / raw)
To: qemu-devel
Cc: Ben Widawsky, Eduardo Habkost, Michael S. Tsirkin, Paolo Bonzini,
Igor Mammedov, Richard Henderson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts
2020-10-26 19:39 ` [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts Ben Widawsky
@ 2020-10-27 14:36 ` Igor Mammedov
2020-10-27 15:45 ` Ben Widawsky
0 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2020-10-27 14:36 UTC (permalink / raw)
To: Ben Widawsky
Cc: Eduardo Habkost, mst, jusual, qemu-devel, Paolo Bonzini,
Richard Henderson
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);
> + }
> }
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts
2020-10-27 14:36 ` Igor Mammedov
@ 2020-10-27 15:45 ` Ben Widawsky
2020-10-29 13:57 ` Igor Mammedov
0 siblings, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2020-10-27 15:45 UTC (permalink / raw)
To: Igor Mammedov
Cc: Eduardo Habkost, mst, jusual, qemu-devel, Paolo Bonzini,
Richard Henderson
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);
> > + }
> > }
> > }
> >
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts
2020-10-27 15:45 ` Ben Widawsky
@ 2020-10-29 13:57 ` Igor Mammedov
0 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2020-10-29 13:57 UTC (permalink / raw)
To: Ben Widawsky
Cc: Eduardo Habkost, mst, jusual, qemu-devel, Paolo Bonzini,
Richard Henderson
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.
[...]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges
2020-10-26 19:39 [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges Ben Widawsky
2020-10-26 19:39 ` [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts Ben Widawsky
@ 2020-10-29 13:59 ` Igor Mammedov
1 sibling, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2020-10-29 13:59 UTC (permalink / raw)
To: Ben Widawsky
Cc: Paolo Bonzini, Richard Henderson, Michael S. Tsirkin, qemu-devel,
Eduardo Habkost
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,
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-29 14:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-26 19:39 [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges Ben Widawsky
2020-10-26 19:39 ` [PATCH 2/2] acpi/crs: Support ranges > 32b for hosts Ben Widawsky
2020-10-27 14:36 ` Igor Mammedov
2020-10-27 15:45 ` Ben Widawsky
2020-10-29 13:57 ` Igor Mammedov
2020-10-29 13:59 ` [PATCH 1/2] acpi/crs: Prevent bad ranges for host bridges Igor Mammedov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).