On Thu, Jun 20, 2019 at 03:14:04PM +0200, Paolo Bonzini wrote: > On 20/06/19 14:59, Peter Xu wrote: > > I feel like this can be problematic. I'm imaging: > > > > start=0x1000_0000, size=0x1000_1000 > > > > This will get size=0x1000 but actually we can do size=0x1000_0000 as > > the first. > > Right, we can do: > > /* > * If a naturally aligned region starting at "start" ends before "end", > * use it. Otherwise, keep the lowest bit of size. > */ > if (size > (start & -start)) > size = start & -start;
May need to consider start==0, otherwise size will be zero here? > else > size = size & -size; Should use MSB rather than LSB of size? > > >> > >> + trace_vtd_as_unmap_whole(pci_bus_num(as->bus), > >> + VTD_PCI_SLOT(as->devfn), > >> + VTD_PCI_FUNC(as->devfn), > >> + entry.iova, size); > > > > Can move this out because this is a trace only so we don't have > > restriction on mask? > > > >> > >> - map.iova = entry.iova; > >> - map.size = entry.addr_mask; > >> - iova_tree_remove(as->iova_tree, &map); > >> + map.iova = entry.iova; > >> + map.size = entry.addr_mask; > >> + iova_tree_remove(as->iova_tree, &map); > > > > Same here? > > > > Yes, I would move these and the iova_tree_remove outside the loop, while > keeping entry's initialization inside looks cleaner. Yeah that's ok to me too. Thanks, -- Peter Xu
