[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/5] hvmloader: Correct bug in low mmio region accounting
On Thu, 20 Jun 2013, George Dunlap wrote: > On 19/06/13 18:18, Stefano Stabellini wrote: > > On Tue, 18 Jun 2013, George Dunlap wrote: > > > 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. > > > > > > This patch first changes the name of this variable to "low_mmio_left" > > > to distinguish it from generic MMIO, and corrects the logic to only > > > subtract the size of the BAR for devices maped in the low MMIO region. > > > > > > Also make low_mmio_left unsigned, and don't allow it to go negative. > > > Since its main use is to be compared to a 64-bit unsigned int, this > > > may have undefined (and in practice almost certainly incorrect) > > > results. Not subtracting is OK because if there's not enough room, it > > > won't actually be mapped. > > > > > > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx> > > > CC: Ian Jackson <ian.jackson@xxxxxxxxxx> > > > CC: Ian Campbell <ian.campbell@xxxxxxxxxx> > > > CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx> > > > CC: Hanweidong <hanweidong@xxxxxxxxxx> > > > --- > > > tools/firmware/hvmloader/pci.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/firmware/hvmloader/pci.c > > > b/tools/firmware/hvmloader/pci.c > > > index c78d4d3..8691a19 100644 > > > --- a/tools/firmware/hvmloader/pci.c > > > +++ b/tools/firmware/hvmloader/pci.c > > > @@ -38,11 +38,10 @@ void pci_setup(void) > > > { > > > uint8_t is_64bar, using_64bar, bar64_relocate = 0; > > > uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper; > > > - uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0; > > > + uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0; > > > 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 { > > > @@ -244,7 +243,7 @@ void pci_setup(void) > > > io_resource.base = 0xc000; > > > io_resource.max = 0x10000; > > > - mmio_left = pci_mem_end - pci_mem_start; > > > + low_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++ ) > > > @@ -253,7 +252,7 @@ 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); > > > + using_64bar = bars[i].is_64bar && bar64_relocate && > > > (low_mmio_left < bar_sz); > > > bar_data = pci_readl(devfn, bar_reg); > > > if ( (bar_data & PCI_BASE_ADDRESS_SPACE) == > > > @@ -273,9 +272,10 @@ void pci_setup(void) > > > } > > > else { > > > resource = &mem_resource; > > > + if ( bar_sz <= low_mmio_left ) > > > + low_mmio_left -= bar_sz; > > Why do you need this check? Isn't the above if(using_64bar && (bar_sz > > > PCI_MIN_BIG_BAR_SIZE)) enough? > > This is in the lowmem region. There may be regions which can't be relocated > to the high PCI region that nevertheless don't fit in the low PCI region. If > it doesn't fit, it will hit the "no space for resource" conditional below and > not be mapped; we need to make sure not to subtract it off. > > I suppose a more robust method might be to use resource->max - resource->base > instead of keeping a separate accounting... I had originally thought that > would be too invasive a change, but I'm not so sure now... any thoughts? You could just add: if (resource == &mem_resource) low_mmio_left -= bar_sz; right below the resource size check. This way we would have only one check to see if the bar fits. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |