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