[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting
George Dunlap writes ("[PATCH v4 5/8] 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. ... > - using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left < > bar_sz); > + using_64bar = bars[i].is_64bar && bar64_relocate > + && (bar_sz > (mem_resource.max - mem_resource.base)); This is not entirely straightforward I think. The actual calculation about whether it will actually fit, rather than a precalculation of whether it is going to fit, is done here: base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1); bar_data |= (uint32_t)base; bar_data_upper = (uint32_t)(base >> 32); base += bar_sz; if ( (base < resource->base) || (base > resource->max) ) [ ... doesn't fit ... ] The first test rounds the base up to a multiple of bar_sz. I assume that this is a requirement of the PCI spec. (While I'm here I'll note that the (uint64_t) cast in that line is unneccessary and confusing. If bar_sz weren't 64-bit this code would be quite wrong, and putting that cast there suggests that it might not be.) In infer (from "bar_sz &= ~(bar_sz - 1)") that bar_sz is supposed to be always a power of two. And we have devices in descending order of size. So at least after the first device, this rounding does nothing. But for the first device I think it may be possible for resource->base not to be a multiple of the bar_sz, and in that case it might be that the precalculation thinks it will fit when the actual placement calculation doesn't. Do you think this is possible ? This is certainly excessively confusing. From a lack-of-regressions point of view we are going to have to analyse it properly regardless of whether we restructure it or not. I would be tempted to suggest lifting the "base" etc. calculation into a macro or function so that we can directly say + using_64bar = bars[i].is_64bar && bar64_relocate + && !try_allocate_resource(&mem_resource, &allocd, &new_base) and later - base = (resource->base + bar_sz - 1) & ~(uint64_t)(bar_sz - 1); - bar_data |= (uint32_t)base; - bar_data_upper = (uint32_t)(base >> 32); - base += bar_sz; - + if ( !try_allocate_resource(resource, &allocd, &new_base) ) { printf("pci dev %02x:%x bar %02x size "PRIllx": no space for " "resource!\n", devfn>>3, devfn&7, bar_reg, PRIllx_arg(bar_sz)); continue; } - resource->base = base; + resource->base = new_base; + bar_data |= (uint32_t)allocd; + bar_data_upper = (uint32_t)(allocd >> 32); or something. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |