diff options
author | George Dunlap <george.dunlap@eu.citrix.com> | 2013-06-18 15:11:03 +0100 |
---|---|---|
committer | Ian Jackson <Ian.Jackson@eu.citrix.com> | 2013-06-24 12:20:29 +0100 |
commit | 703e150231e26079d72ecca76701a825e7b9b927 (patch) | |
tree | 3419aa781db8f4cce31348a16da2a979acefae52 /tools/firmware | |
parent | 3458ab670f3043100f2c0ae5634c44226eb3d2ca (diff) | |
download | xen-703e150231e26079d72ecca76701a825e7b9b927.tar.gz xen-703e150231e26079d72ecca76701a825e7b9b927.tar.bz2 xen-703e150231e26079d72ecca76701a825e7b9b927.zip |
hvmloader: Correct bug in low mmio region accounting
When deciding whether to map a device in low MMIO space (<4GiB),
hvmloader compares it with "mmio_left", which is set to the size of
the low MMIO range (pci_mem_end - pci_mem_start). However, even if it
does map a device in high MMIO space, it still removes the size of its
BAR from mmio_left.
In reality we don't need to do a separate accounting of the low memory
available -- this can be calculated from mem_resource. Just get rid
of the variable and the duplicate accounting entirely. This will make
the code more robust.
Note also that the calculation of whether to move a device to 64-bit
is fragile at the moment, depending on some unstated assumptions.
State those assumptions in a comment for future reference.
v5:
- Add comment documenting fragility of the move-to-highmem check
v3:
- Use mem_resource values directly instead of doing duplicate
accounting
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Hanweidong <hanweidong@huawei.com>
CC: Keir Fraser <keir@xen.org>
Diffstat (limited to 'tools/firmware')
-rw-r--r-- | tools/firmware/hvmloader/pci.c | 20 |
1 files changed, 15 insertions, 5 deletions
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c index 6792ed47a4..1fe250ddc7 100644 --- a/tools/firmware/hvmloader/pci.c +++ b/tools/firmware/hvmloader/pci.c @@ -42,7 +42,6 @@ void pci_setup(void) uint32_t vga_devfn = 256; uint16_t class, vendor_id, device_id; unsigned int bar, pin, link, isa_irq; - int64_t mmio_left; /* Resources assignable to PCI devices via BARs. */ struct resource { @@ -264,8 +263,6 @@ void pci_setup(void) io_resource.base = 0xc000; io_resource.max = 0x10000; - mmio_left = pci_mem_end - pci_mem_start; - /* Assign iomem and ioport resources in descending order of size. */ for ( i = 0; i < nr_bars; i++ ) { @@ -273,7 +270,21 @@ void pci_setup(void) bar_reg = bars[i].bar_reg; bar_sz = bars[i].bar_sz; - using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left < bar_sz); + /* + * NB: The code here is rather fragile, as the check here to see + * whether bar_sz will fit in the low MMIO region doesn't match the + * real check made below, which involves aligning the base offset of the + * bar with the size of the bar itself. As it happens, this will always + * be satisfied because: + * - The first one will succeed because the MMIO hole can only start at + * 0x{f,e,c,8}00000000. If it fits, it will be aligned properly. + * - All subsequent ones will be aligned because the list is ordered + * large to small, and bar_sz is always a power of 2. (At least + * the code here assumes it to be.) + * Should either of those two conditions change, this code will break. + */ + using_64bar = bars[i].is_64bar && bar64_relocate + && (bar_sz > (mem_resource.max - mem_resource.base)); bar_data = pci_readl(devfn, bar_reg); if ( (bar_data & PCI_BASE_ADDRESS_SPACE) == @@ -295,7 +306,6 @@ void pci_setup(void) resource = &mem_resource; bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK; } - mmio_left -= bar_sz; } else { |