[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [v3][PATCH 07/16] hvmloader/pci: skip reserved ranges
- To: "Tian, Kevin" <kevin.tian@xxxxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, "tim@xxxxxxx" <tim@xxxxxxx>, "andrew.cooper3@xxxxxxxxxx" <andrew.cooper3@xxxxxxxxxx>, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>, "wei.liu2@xxxxxxxxxx" <wei.liu2@xxxxxxxxxx>, "ian.campbell@xxxxxxxxxx" <ian.campbell@xxxxxxxxxx>, "Ian.Jackson@xxxxxxxxxxxxx" <Ian.Jackson@xxxxxxxxxxxxx>, "stefano.stabellini@xxxxxxxxxx" <stefano.stabellini@xxxxxxxxxx>
- From: "Chen, Tiejun" <tiejun.chen@xxxxxxxxx>
- Date: Fri, 12 Jun 2015 15:53:36 +0800
- Cc: "xen-devel@xxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxx>
- Delivery-date: Fri, 12 Jun 2015 07:53:58 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 2015/6/11 17:51, Tian, Kevin wrote:
From: Chen, Tiejun
Sent: Thursday, June 11, 2015 9:15 AM
When allocating mmio address for PCI bars, we need to make
sure they don't overlap with reserved regions.
Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx>
---
tools/firmware/hvmloader/pci.c | 36
++++++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 5ff87a7..98af568 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -59,8 +59,8 @@ void pci_setup(void)
uint32_t bar_reg;
uint64_t bar_sz;
} *bars = (struct bars *)scratch_start;
- unsigned int i, nr_bars = 0;
- uint64_t mmio_hole_size = 0;
+ unsigned int i, j, nr_bars = 0;
+ uint64_t mmio_hole_size = 0, reserved_end, max_bar_sz = 0;
const char *s;
/*
@@ -226,6 +226,8 @@ void pci_setup(void)
bars[i].devfn = devfn;
bars[i].bar_reg = bar_reg;
bars[i].bar_sz = bar_sz;
+ if ( bar_sz > max_bar_sz )
+ max_bar_sz = bar_sz;
if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
PCI_BASE_ADDRESS_SPACE_MEMORY) ||
@@ -301,6 +303,21 @@ void pci_setup(void)
pci_mem_start <<= 1;
}
+ /* Relocate PCI memory that overlaps reserved space, like RDM. */
+ for ( j = 0; j < memory_map.nr_map ; j++ )
+ {
+ if ( memory_map.map[j].type != E820_RAM )
+ {
+ reserved_end = memory_map.map[j].addr + memory_map.map[j].size;
+ if ( check_overlap(pci_mem_start, pci_mem_end,
+ memory_map.map[j].addr,
+ memory_map.map[j].size) )
+ pci_mem_start -= memory_map.map[j].size >> PAGE_SHIFT;
what's the point of subtracting reserved size here? I think you want
to move pci_mem_start higher instead of lower to avoid conflict, right?
No.
+ pci_mem_start = (pci_mem_start + max_bar_sz - 1) &
+ ~(uint64_t)(max_bar_sz - 1);
better have some comment to explain what exactly you're trying to
achieve here.
Actually I didn't have this code fragment. Here I add this chunk of
codes to address one concern that Jan raised. Please see the below.
+ }
+ }
+
if ( mmio_total > (pci_mem_end - pci_mem_start) )
{
printf("Low MMIO hole not large enough for all devices,"
@@ -407,8 +424,23 @@ void pci_setup(void)
}
base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
+ reallocate_mmio:
In earlier comment you said:
+ /* Relocate PCI memory that overlaps reserved space, like RDM. */
If pci_mem_start has been relocated to avoid overlapping, how will actual
allocation here will conflict again? Sorry I may miss the two relocations
here...
bar_data |= (uint32_t)base;
bar_data_upper = (uint32_t)(base >> 32);
+ for ( j = 0; j < memory_map.nr_map ; j++ )
+ {
+ if ( memory_map.map[j].type != E820_RAM )
+ {
+ reserved_end = memory_map.map[j].addr +
memory_map.map[j].size;
+ if ( check_overlap(base, bar_sz,
+ memory_map.map[j].addr,
+ memory_map.map[j].size) )
+ {
+ base = (reserved_end + bar_sz - 1) & ~(uint64_t)(bar_sz -
1);
+ goto reallocate_mmio;
That is because our previous implementation is just skipping that
conflict region,
"But you do nothing to make sure the MMIO regions all fit in the
available window (see the code ahead of this relocating RAM if
necessary)." and "...it simply skips assigning resources. Your changes
potentially growing the space needed to fit all MMIO BARs therefore also
needs to adjust the up front calculation, such that if necessary more
RAM can be relocated to make the hole large enough."
And then I replied as follows,
"You're right.
Just think about we're always trying to check pci_mem_start to populate
more RAM to obtain enough PCI mempry,
/* Relocate RAM that overlaps PCI space (in 64k-page chunks). */
while ( (pci_mem_start >> PAGE_SHIFT) < hvm_info->low_mem_pgend )
{
struct xen_add_to_physmap xatp;
unsigned int nr_pages = min_t(
unsigned int,
hvm_info->low_mem_pgend - (pci_mem_start >> PAGE_SHIFT),
(1u << 16) - 1);
if ( hvm_info->high_mem_pgend == 0 )
hvm_info->high_mem_pgend = 1ull << (32 - PAGE_SHIFT);
hvm_info->low_mem_pgend -= nr_pages;
printf("Relocating 0x%x pages from "PRIllx" to "PRIllx\
" for lowmem MMIO hole\n",
nr_pages,
PRIllx_arg(((uint64_t)hvm_info->low_mem_pgend)<<PAGE_SHIFT),
PRIllx_arg(((uint64_t)hvm_info->high_mem_pgend)<<PAGE_SHIFT));
xatp.domid = DOMID_SELF;
xatp.space = XENMAPSPACE_gmfn_range;
xatp.idx = hvm_info->low_mem_pgend;
xatp.gpfn = hvm_info->high_mem_pgend;
xatp.size = nr_pages;
if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
BUG();
hvm_info->high_mem_pgend += nr_pages;
}
"
I hope this can help you understand this background. And I will update
that code comment like this,
/*
* We'll skip all space overlapping with reserved memory later,
* so we need to decrease pci_mem_start to populate more RAM
* to compensate them.
*/
Thanks
Tiejun
+ }
+ }
+ }
base += bar_sz;
if ( (base < resource->base) || (base > resource->max) )
--
1.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
