Re: [Xen-devel] [v11][PATCH 06/16] hvmloader/pci: Try to avoid placing BARs in RMRRs
>>> On 22.07.15 at 10:43, wrote:
On 22.07.15 at 03:29, wrote:
> > --- a/tools/firmware/hvmloader/pci.c
> > +++ b/tools/firmware/hvmloader/pci.c
> > @@ -38,6 +38,46 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
> > enum virtual_vga virtual_vga = VGA_none;
> > unsigned long igd_opregion_pgbase = 0;
> >
> > +/* Check if the specified range conflicts with any reserved device memory.
> */
> > +static bool check_overlap_all(uint64_t start, uint64_t size)
> > +{
> > +unsigned int i;
> > +
> > +for ( i = 0; i < memory_map.nr_map; i++ )
> > +{
> > +if ( memory_map.map[i].type == E820_RESERVED &&
> > + check_overlap(start, size,
> > + memory_map.map[i].addr,
> > + memory_map.map[i].size) )
> > +return true;
> > +}
> > +
> > +return false;
> > +}
> > +
> > +/* Find the lowest RMRR higher than base. */
>
> This comment should have been updated; I'm doing this for you in
> anticipation of this going in later today.
>
> > +static int find_next_rmrr(uint32_t base)
> > +{
> > +unsigned int i;
> > +int next_rmrr = -1;
> > +uint64_t end, min_end = (1ull << 32);
> > +
> > +for ( i = 0; i < memory_map.nr_map ; i++ )
> > +{
> > +end = memory_map.map[i].addr + memory_map.map[i].size;
> > +
> > +if ( memory_map.map[i].type == E820_RESERVED &&
> > + end > base &&
> > + min_end < min_end )
>
> Surely "end < min_end"?
>
> Or really I think this part should be concerned about the start of
> the region, albeit it probably doesn't matter much since right
> below 4G there shouldn't be an RMRR anyway. Just to be on the
> safe side I'll at least make it "end <= min_end".
I.e like the below (also attached, just in case); I hope/think I didn't
do any other edits to further patches.
Jan
hvmloader/pci: try to avoid placing BARs in RMRRs
Try to avoid placing PCI BARs over RMRRs:
- If mmio_hole_size is not specified, and the existing MMIO range has
RMRRs in it, and there is space to expand the hole in lowmem without
moving more memory, then make the MMIO hole as large as possible.
- When placing RMRRs, find the next RMRR higher than the current base
in the lowmem mmio hole. If it overlaps, skip ahead of it and find
the next one.
This certainly won't work in all cases, but it should work in a
significant number of cases. Additionally, users should be able to
work around problems by setting mmio_hole_size larger in the guest
config.
Signed-off-by: George Dunlap
Signed-off-by: Tiejun Chen
Reviewed-by: Jan Beulich
Release-acked-by: Wei Liu
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -38,6 +38,45 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
enum virtual_vga virtual_vga = VGA_none;
unsigned long igd_opregion_pgbase = 0;
+/* Check if the specified range conflicts with any reserved device memory. */
+static bool check_overlap_all(uint64_t start, uint64_t size)
+{
+unsigned int i;
+
+for ( i = 0; i < memory_map.nr_map; i++ )
+{
+if ( memory_map.map[i].type == E820_RESERVED &&
+ check_overlap(start, size,
+ memory_map.map[i].addr,
+ memory_map.map[i].size) )
+return true;
+}
+
+return false;
+}
+
+/* Find the lowest RMRR ending above base but below 4G. */
+static int find_next_rmrr(uint32_t base)
+{
+unsigned int i;
+int next_rmrr = -1;
+uint64_t end, min_end = 1ULL << 32;
+
+for ( i = 0; i < memory_map.nr_map ; i++ )
+{
+end = memory_map.map[i].addr + memory_map.map[i].size;
+
+if ( memory_map.map[i].type == E820_RESERVED &&
+ end > base && end <= min_end )
+{
+next_rmrr = i;
+min_end = end;
+}
+}
+
+return next_rmrr;
+}
+
void pci_setup(void)
{
uint8_t is_64bar, using_64bar, bar64_relocate = 0;
@@ -46,6 +85,7 @@ void pci_setup(void)
uint32_t vga_devfn = 256;
uint16_t class, vendor_id, device_id;
unsigned int bar, pin, link, isa_irq;
+int next_rmrr;
/* Resources assignable to PCI devices via BARs. */
struct resource {
@@ -299,6 +339,15 @@ void pci_setup(void)
|| (((pci_mem_start << 1) >> PAGE_SHIFT)
>= hvm_info->low_mem_pgend)) )
pci_mem_start <<= 1;
+
+/*
+ * Try to accommodate RMRRs in our MMIO region on a best-effort basis.
+ * If we have RMRRs in the range, then make pci_mem_start just after
+ * hvm_info->low_mem_pgend.
+ */
+if ( pci_mem_start > (hvm_info->low_mem_pgend << PAGE_SHIFT) &&
+ check_overlap_all(pci_mem_start, pci_mem_end-pci_mem_start) )
+pci_mem_start = hvm_info->low_mem_pgend << PAGE_SHIFT;
}
if ( mmio_total > (pci_mem_end - pci_mem_start) )
@@ -352,6 +401,8 @@ void pci_setup(void)
io_resource.base = 0
Re: [Xen-devel] [v11][PATCH 06/16] hvmloader/pci: Try to avoid placing BARs in RMRRs
>>> On 22.07.15 at 03:29, wrote:
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -38,6 +38,46 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
> enum virtual_vga virtual_vga = VGA_none;
> unsigned long igd_opregion_pgbase = 0;
>
> +/* Check if the specified range conflicts with any reserved device memory. */
> +static bool check_overlap_all(uint64_t start, uint64_t size)
> +{
> +unsigned int i;
> +
> +for ( i = 0; i < memory_map.nr_map; i++ )
> +{
> +if ( memory_map.map[i].type == E820_RESERVED &&
> + check_overlap(start, size,
> + memory_map.map[i].addr,
> + memory_map.map[i].size) )
> +return true;
> +}
> +
> +return false;
> +}
> +
> +/* Find the lowest RMRR higher than base. */
This comment should have been updated; I'm doing this for you in
anticipation of this going in later today.
> +static int find_next_rmrr(uint32_t base)
> +{
> +unsigned int i;
> +int next_rmrr = -1;
> +uint64_t end, min_end = (1ull << 32);
> +
> +for ( i = 0; i < memory_map.nr_map ; i++ )
> +{
> +end = memory_map.map[i].addr + memory_map.map[i].size;
> +
> +if ( memory_map.map[i].type == E820_RESERVED &&
> + end > base &&
> + min_end < min_end )
Surely "end < min_end"?
Or really I think this part should be concerned about the start of
the region, albeit it probably doesn't matter much since right
below 4G there shouldn't be an RMRR anyway. Just to be on the
safe side I'll at least make it "end <= min_end".
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [v11][PATCH 06/16] hvmloader/pci: Try to avoid placing BARs in RMRRs
Try to avoid placing PCI BARs over RMRRs:
- If mmio_hole_size is not specified, and the existing MMIO range has
RMRRs in it, and there is space to expand the hole in lowmem without
moving more memory, then make the MMIO hole as large as possible.
- When placing RMRRs, find the next RMRR higher than the current base
in the lowmem mmio hole. If it overlaps, skip ahead of it and find
the next one.
This certainly won't work in all cases, but it should work in a
significant number of cases. Additionally, users should be able to
work around problems by setting mmio_hole_size larger in the guest
config.
CC: Keir Fraser
CC: Jan Beulich
CC: Andrew Cooper
CC: Ian Jackson
CC: Stefano Stabellini
CC: Ian Campbell
CC: Wei Liu
Reviewed-by: Jan Beulich
Signed-off-by: George Dunlap
Signed-off-by: Tiejun Chen
---
v11:
* To find the lowest RMRR the _end_ of which is higher than base.
* Refine some code implementations
v10:
* This is from George' draft patch which implements an acceptable solution in
current cycle. Here I just implemented check_overlap_all() and some cleanups.
v9:
* A little improvement to code implementation but again, its still argued about
this solution.
v8:
* Based on current discussion its hard to reshape the original mmio
allocation mechanism but we haven't a good and simple way to in short term.
So instead, we don't bring more complicated to intervene that process but
still check any conflicts to disable all associated devices.
v6 ~ v7:
* Nothing is changed.
v5:
* Rename that field, is_64bar, inside struct bars with flag, and
then extend to also indicate if this bar is already allocated.
v4:
* We have to re-design this as follows:
#1. Goal
MMIO region should exclude all reserved device memory
#2. Requirements
#2.1 Still need to make sure MMIO region is fit all pci devices as before
#2.2 Accommodate the not aligned reserved memory regions
If I'm missing something let me know.
#3. How to
#3.1 Address #2.1
We need to either of populating more RAM, or of expanding more highmem. But
we should know just 64bit-bar can work with highmem, and as you mentioned we
also should avoid expanding highmem as possible. So my implementation is to
allocate 32bit-bar and 64bit-bar orderly.
1>. The first allocation round just to 32bit-bar
If we can finish allocating all 32bit-bar, we just go to allocate 64bit-bar
with all remaining resources including low pci memory.
If not, we need to calculate how much RAM should be populated to allocate the
remaining 32bit-bars, then populate sufficient RAM as exp_mem_resource to go
to the second allocation round 2>.
2>. The second allocation round to the remaining 32bit-bar
We should can finish allocating all 32bit-bar in theory, then go to the third
allocation round 3>.
3>. The third allocation round to 64bit-bar
We'll try to first allocate from the remaining low memory resource. If that
isn't enough, we try to expand highmem to allocate for 64bit-bar. This process
should be same as the original.
#3.2 Address #2.2
I'm trying to accommodate the not aligned reserved memory regions:
We should skip all reserved device memory, but we also need to check if other
smaller bars can be allocated if a mmio hole exists between resource->base and
reserved device memory. If a hole exists between base and reserved device
memory, lets go out simply to try allocate for next bar since all bars are in
descending order of size. If not, we need to move resource->base to
reserved_end
just to reallocate this bar.
tools/firmware/hvmloader/pci.c | 65 ++
1 file changed, 65 insertions(+)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 5ff87a7..74fc080 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -38,6 +38,46 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
enum virtual_vga virtual_vga = VGA_none;
unsigned long igd_opregion_pgbase = 0;
+/* Check if the specified range conflicts with any reserved device memory. */
+static bool check_overlap_all(uint64_t start, uint64_t size)
+{
+unsigned int i;
+
+for ( i = 0; i < memory_map.nr_map; i++ )
+{
+if ( memory_map.map[i].type == E820_RESERVED &&
+ check_overlap(start, size,
+ memory_map.map[i].addr,
+ memory_map.map[i].size) )
+return true;
+}
+
+return false;
+}
+
+/* Find the lowest RMRR higher than base. */
+static int find_next_rmrr(uint32_t base)
+{
+unsigned int i;
+int next_rmrr = -1;
+uint64_t end, min_end = (1ull << 32);
+
+for ( i = 0; i < memory_map.nr_map ; i++ )
+{
+end = memory_map.map[i].addr + memory_map.map[i].size;
+
+if ( memory_map.map[i].type == E820_RESERVED &&
+ end > base &&
+ min_end < min_end )
+{
+next
