| Message ID | 56CBBFD7.3050806@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 23, 2016 at 01:11:35PM +1100, Alexey Kardashevskiy wrote: > On 02/22/2016 11:12 PM, David Gibson wrote: > >On Mon, Feb 22, 2016 at 10:36:19PM +1100, Alexey Kardashevskiy wrote: > >>On 02/22/2016 05:26 PM, David Gibson wrote: > >>>On Mon, Feb 22, 2016 at 05:09:39PM +1100, Alexey Kardashevskiy wrote: > >>>>Since a788f227 "memory: Allow replay of IOMMU mapping notifications" > >>>>when new VFIO listener is added, all existing IOMMU mappings are replayed. > >>>>However there is a problem that the base address of an IOMMU memory region > >>>>(IOMMU MR) is ignored which is not a problem for the existing user (which is > >>>>pseries) with its default 32bit DMA window starting at 0 but it is if there is > >>>>another DMA window. > >>>> > >>>>This adjusts the replaying address by mr->addr. > >>> > >>>Uh.. this doesn't look right to me. AFAICT from the existing > >>>implementations the 'addr' parameter to the translate function is an > >>>offset within the memory region, which would make the original version > >>>correct. > >> > >>Ok, then spapr_tce_translate_iommu() needs to be fixed. Or I am missing > >>something here? The @addr field is definitely ignored now. > > > >I think at least one of us is missing something. In the normal AS > >translation path, IIUC, it will subtract the mr->addr value to give > >offsets which are passed to translate. It uses those offsets to find > >the TCE and returns a translated address. Where else the @addr be > >used? > > Ok, this patch is definitely wrong. > > The actual problem I am debugging is this: > 1. run guest with an emulated XHCI to have 2 DMA windows; > 2. hotplug vfio-pci device. > 3. observe failing vfio_dma_map(). > > This triggers memory_region_register_iommu_notifier() + > memory_region_iommu_replay() which calls vfio_iommu_map_notify() in a loop. > For the second window (huge one, 0x800000000000000 bus address) it fails as > iotlb.iova's returned by spapr_tce_translate_iommu() are offsets within > IOMMU MR. > > But vfio_dma_map() (which is eventually called by vfio_iommu_map_notify()) > needs an absolute iotlb->iova address. > > imho this should be correct: > > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 2e02dc6..0d6b926 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -126,7 +126,7 @@ static IOMMUTLBEntry > spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, > hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); > > tce = tcet->table[addr >> tcet->page_shift]; > - ret.iova = addr & page_mask; > + ret.iova = (addr + iommu->addr) & page_mask; > ret.translated_addr = tce & page_mask; I wondered about that change, but I'd have to look closer to see if the iova field here is expected to be relative to the MR as well. It would be oddly inconsistent if it wasn't. > However this does not help either. > > Because at the moment I create hardware windows from a helper which is > called from spapr_phb_hot_plug_child() (HotplugHandlerClass::plug() > callback). And this is too late as pci_qdev_realize("vfio-pci") calls: > > vfio_connect_container -> ... -> vfio_listener_region_add -> .. -> > memory_region_iommu_replay > and the latter fails - there is no huge window in the host kernel yet. > What is the right way of fixing this?... Hmm.. I think we need a backend specific hook which will be called from vfio_listener_region_add() when it sees a new iommu region, but before it calls replay. Basically that notifier is enumerating to the VFIO subsystem where the guest has IOMMU windows. With DDW on the host, we need something to mirror the guest side windows into the host (or reject them, if they're not compatible. Actually.. that hook should replace the test a little bit higher testing against container->min_iova and container->max_iova. With host-side DDW the set of allowable IOVAs is no longer a single, simple range - and it's now changeable.
On 02/23/2016 05:20 PM, David Gibson wrote: > On Tue, Feb 23, 2016 at 01:11:35PM +1100, Alexey Kardashevskiy wrote: >> On 02/22/2016 11:12 PM, David Gibson wrote: >>> On Mon, Feb 22, 2016 at 10:36:19PM +1100, Alexey Kardashevskiy wrote: >>>> On 02/22/2016 05:26 PM, David Gibson wrote: >>>>> On Mon, Feb 22, 2016 at 05:09:39PM +1100, Alexey Kardashevskiy wrote: >>>>>> Since a788f227 "memory: Allow replay of IOMMU mapping notifications" >>>>>> when new VFIO listener is added, all existing IOMMU mappings are replayed. >>>>>> However there is a problem that the base address of an IOMMU memory region >>>>>> (IOMMU MR) is ignored which is not a problem for the existing user (which is >>>>>> pseries) with its default 32bit DMA window starting at 0 but it is if there is >>>>>> another DMA window. >>>>>> >>>>>> This adjusts the replaying address by mr->addr. >>>>> >>>>> Uh.. this doesn't look right to me. AFAICT from the existing >>>>> implementations the 'addr' parameter to the translate function is an >>>>> offset within the memory region, which would make the original version >>>>> correct. >>>> >>>> Ok, then spapr_tce_translate_iommu() needs to be fixed. Or I am missing >>>> something here? The @addr field is definitely ignored now. >>> >>> I think at least one of us is missing something. In the normal AS >>> translation path, IIUC, it will subtract the mr->addr value to give >>> offsets which are passed to translate. It uses those offsets to find >>> the TCE and returns a translated address. Where else the @addr be >>> used? >> >> Ok, this patch is definitely wrong. >> >> The actual problem I am debugging is this: >> 1. run guest with an emulated XHCI to have 2 DMA windows; >> 2. hotplug vfio-pci device. >> 3. observe failing vfio_dma_map(). >> >> This triggers memory_region_register_iommu_notifier() + >> memory_region_iommu_replay() which calls vfio_iommu_map_notify() in a loop. >> For the second window (huge one, 0x800000000000000 bus address) it fails as >> iotlb.iova's returned by spapr_tce_translate_iommu() are offsets within >> IOMMU MR. >> >> But vfio_dma_map() (which is eventually called by vfio_iommu_map_notify()) >> needs an absolute iotlb->iova address. >> >> imho this should be correct: >> >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >> index 2e02dc6..0d6b926 100644 >> --- a/hw/ppc/spapr_iommu.c >> +++ b/hw/ppc/spapr_iommu.c >> @@ -126,7 +126,7 @@ static IOMMUTLBEntry >> spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, >> hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); >> >> tce = tcet->table[addr >> tcet->page_shift]; >> - ret.iova = addr & page_mask; >> + ret.iova = (addr + iommu->addr) & page_mask; >> ret.translated_addr = tce & page_mask; > > I wondered about that change, but I'd have to look closer to see if > the iova field here is expected to be relative to the MR as well. It > would be oddly inconsistent if it wasn't. It is relative and it does not make sense as there is no source MR/AS in iotlb (only target AS) so there is no use in such iova. > >> However this does not help either. >> >> Because at the moment I create hardware windows from a helper which is >> called from spapr_phb_hot_plug_child() (HotplugHandlerClass::plug() >> callback). And this is too late as pci_qdev_realize("vfio-pci") calls: >> >> vfio_connect_container -> ... -> vfio_listener_region_add -> .. -> >> memory_region_iommu_replay >> and the latter fails - there is no huge window in the host kernel yet. >> What is the right way of fixing this?... > > Hmm.. I think we need a backend specific hook which will be called > from vfio_listener_region_add() when it sees a new iommu region, but > before it calls replay. > > Basically that notifier is enumerating to the VFIO subsystem where the > guest has IOMMU windows. With DDW on the host, we need something to > mirror the guest side windows into the host (or reject them, if > they're not compatible. The host kernel API consists of 2 ioctls basically - create and remove a window. There is no "query" which would tell how many windows are there and their parameters. When there is a first vfio-pci device, I basically go through the list of emulated windows and create hardware windows for every active window. But I do this from spapr-phb code where I have the complete picture - how many windows, LIOBNs. This vfio_listener_region_add() thing - it is called per IOMMU MR, i.e. once or twice per PHB, in undefined order, not good (however the order never changes in the reality). And how exactly do I connect VFIO container and spapr PHB (presumably using QOM)? I cannot think of any good interface now... Have another memory listener on PHB's AS in spapr_pci? > Actually.. that hook should replace the test a little bit higher > testing against container->min_iova and container->max_iova. With > host-side DDW the set of allowable IOVAs is no longer a single, simple > range - and it's now changeable. I am missing the purpose of container->min_iova/max_iova, on x86 these are constant values and on pseries - QEMU chooses the windows parameters when creates the windows so this check is more like assert...
On 23/02/2016 10:00, Alexey Kardashevskiy wrote: >>> >>> tce = tcet->table[addr >> tcet->page_shift]; >>> - ret.iova = addr & page_mask; >>> + ret.iova = (addr + iommu->addr) & page_mask; >>> ret.translated_addr = tce & page_mask; >> >> I wondered about that change, but I'd have to look closer to see if >> the iova field here is expected to be relative to the MR as well. It >> would be oddly inconsistent if it wasn't. > > It is relative and it does not make sense as there is no source MR/AS in > iotlb (only target AS) so there is no use in such iova. ret.iova should be relative to the source AS (i.e. even if a 32-bit IOMMU region translates between 4GB and 8GB, ret.iova should have bits 32-63 set to 0). So there is a problem in vfio_iommu_map_notify: ret = vfio_dma_map(container, iotlb->iova, iotlb->addr_mask + 1, vaddr, !(iotlb->perm & IOMMU_WO) || mr->readonly); I think that, in vfio_listener_region_add, the iova variable should be stored in VFIOGuestIOMMU for use in vfio_iommu_map_notify. ret.translated_addr should be relative to the target AS, which VFIO assumes to be address_space_memory. Paolo
On Tue, Feb 23, 2016 at 10:56:57AM +0100, Paolo Bonzini wrote: > > > On 23/02/2016 10:00, Alexey Kardashevskiy wrote: > >>> > >>> tce = tcet->table[addr >> tcet->page_shift]; > >>> - ret.iova = addr & page_mask; > >>> + ret.iova = (addr + iommu->addr) & page_mask; > >>> ret.translated_addr = tce & page_mask; > >> > >> I wondered about that change, but I'd have to look closer to see if > >> the iova field here is expected to be relative to the MR as well. It > >> would be oddly inconsistent if it wasn't. > > > > It is relative and it does not make sense as there is no source MR/AS in > > iotlb (only target AS) so there is no use in such iova. > > ret.iova should be relative to the source AS (i.e. even if a 32-bit > IOMMU region translates between 4GB and 8GB, ret.iova should have bits > 32-63 set to 0). Uh.. relative to the source AS, or the source MR? At the moment spapr implements it relative to the MR.. which seems to match your example. > So there is a problem in vfio_iommu_map_notify: > > ret = vfio_dma_map(container, iotlb->iova, > iotlb->addr_mask + 1, vaddr, > !(iotlb->perm & IOMMU_WO) || mr->readonly); Ah.. yeah.. this needs the address relative to the AS, not the MR > I think that, in vfio_listener_region_add, the iova variable should be > stored in VFIOGuestIOMMU for use in vfio_iommu_map_notify. Hmm, seems a bit clunky, though I guess it's safe due to the BQL. > ret.translated_addr should be relative to the target AS, which VFIO > assumes to be address_space_memory. Right, and as far as I can tell that is the case.
On 02/23/2016 08:56 PM, Paolo Bonzini wrote: > > > On 23/02/2016 10:00, Alexey Kardashevskiy wrote: >>>> >>>> tce = tcet->table[addr >> tcet->page_shift]; >>>> - ret.iova = addr & page_mask; >>>> + ret.iova = (addr + iommu->addr) & page_mask; >>>> ret.translated_addr = tce & page_mask; >>> >>> I wondered about that change, but I'd have to look closer to see if >>> the iova field here is expected to be relative to the MR as well. It >>> would be oddly inconsistent if it wasn't. >> >> It is relative and it does not make sense as there is no source MR/AS in >> iotlb (only target AS) so there is no use in such iova. > > ret.iova should be relative to the source AS (i.e. even if a 32-bit > IOMMU region translates between 4GB and 8GB, ret.iova should have bits > 32-63 set to 0). In my test branch with 2 DMA windows I have such PHB AS: address-space: pci@800000020000000 0000000000000000-ffffffffffffffff (prio 0, RW): pci@800000020000000.iommu-root 0000000000000000-ffffffffffffffff (prio 0, RW): tce-root-80000001 0800000000000000-08000000ffffffff (prio 0, RW): tce-iommu-80000001 0000000000000000-ffffffffffffffff (prio 0, RW): tce-root-80000000 0000000000000000-000000003fffffff (prio 0, RW): tce-iommu-80000000 0000040000000000-000004000000ffff (prio 0, RW): msi The source AS is 0..(u64)-1. iotlb.iova from spapr_tce_translate_iommu(tce-root-80000001) will be relative to 0800000000000000 which is not source AS. What do I miss here? > > So there is a problem in vfio_iommu_map_notify: > > ret = vfio_dma_map(container, iotlb->iova, > iotlb->addr_mask + 1, vaddr, > !(iotlb->perm & IOMMU_WO) || mr->readonly); > > I think that, in vfio_listener_region_add, the iova variable should be > stored in VFIOGuestIOMMU for use in vfio_iommu_map_notify. > > ret.translated_addr should be relative to the target AS, which VFIO > assumes to be address_space_memory. That is perfectly fine - there is iotlb.target_as.
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 2e02dc6..0d6b926 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -126,7 +126,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); tce = tcet->table[addr >> tcet->page_shift]; - ret.iova = addr & page_mask; + ret.iova = (addr + iommu->addr) & page_mask; ret.translated_addr = tce & page_mask;