[Cc +Eric] On Tue, 13 Mar 2018 15:53:19 +1100 Alexey Kardashevskiy <a...@ozlabs.ru> wrote:
> On 7/3/18 1:17 pm, Alexey Kardashevskiy wrote: > > On 26/02/18 19:36, Alexey Kardashevskiy wrote: > >> On 19/02/18 13:46, Alexey Kardashevskiy wrote: > >>> On 16/02/18 16:28, David Gibson wrote: > >>>> On Wed, Feb 14, 2018 at 08:55:41AM -0700, Alex Williamson wrote: > >>>>> On Wed, 14 Feb 2018 19:09:16 +1100 > >>>>> Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > >>>>> > >>>>>> On 14/02/18 12:33, David Gibson wrote: > >>>>>>> On Tue, Feb 13, 2018 at 07:20:56PM +1100, Alexey Kardashevskiy wrote: > >>>>>>> > >>>>>>>> On 13/02/18 16:41, David Gibson wrote: > >>>>>>>>> On Tue, Feb 13, 2018 at 04:36:30PM +1100, David Gibson wrote: > >>>>>>>>>> On Tue, Feb 13, 2018 at 12:15:52PM +1100, Alexey Kardashevskiy > >>>>>>>>>> wrote: > >>>>>>>>>>> On 13/02/18 03:06, Alex Williamson wrote: > >>>>>>>>>>>> On Mon, 12 Feb 2018 18:05:54 +1100 > >>>>>>>>>>>> Alexey Kardashevskiy <a...@ozlabs.ru> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>>> On 12/02/18 16:19, David Gibson wrote: > >>>>>>>>>>>>>> On Fri, Feb 09, 2018 at 06:55:01PM +1100, Alexey Kardashevskiy > >>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>> At the moment if vfio_memory_listener is registered in the > >>>>>>>>>>>>>>> system memory > >>>>>>>>>>>>>>> address space, it maps/unmaps every RAM memory region for DMA. > >>>>>>>>>>>>>>> It expects system page size aligned memory sections so > >>>>>>>>>>>>>>> vfio_dma_map > >>>>>>>>>>>>>>> would not fail and so far this has been the case. A mapping > >>>>>>>>>>>>>>> failure > >>>>>>>>>>>>>>> would be fatal. A side effect of such behavior is that some > >>>>>>>>>>>>>>> MMIO pages > >>>>>>>>>>>>>>> would not be mapped silently. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> However we are going to change MSIX BAR handling so we will > >>>>>>>>>>>>>>> end having > >>>>>>>>>>>>>>> non-aligned sections in vfio_memory_listener (more details is > >>>>>>>>>>>>>>> in > >>>>>>>>>>>>>>> the next patch) and vfio_dma_map will exit QEMU. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> In order to avoid fatal failures on what previously was not a > >>>>>>>>>>>>>>> failure and > >>>>>>>>>>>>>>> was just silently ignored, this checks the section alignment > >>>>>>>>>>>>>>> to > >>>>>>>>>>>>>>> the smallest supported IOMMU page size and prints an error if > >>>>>>>>>>>>>>> not aligned; > >>>>>>>>>>>>>>> it also prints an error if vfio_dma_map failed despite the > >>>>>>>>>>>>>>> page size check. > >>>>>>>>>>>>>>> Both errors are not fatal; only MMIO RAM regions are checked > >>>>>>>>>>>>>>> (aka "RAM device" regions). > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> If the amount of errors printed is overwhelming, the MSIX > >>>>>>>>>>>>>>> relocation > >>>>>>>>>>>>>>> could be used to avoid excessive error output. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> This is unlikely to cause any behavioral change. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> There are some relatively superficial problems noted below. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> But more fundamentally, this feels like it's extending an > >>>>>>>>>>>>>> existing > >>>>>>>>>>>>>> hack past the point of usefulness. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> The explicit check for is_ram_device() here has always > >>>>>>>>>>>>>> bothered me - > >>>>>>>>>>>>>> it's not like a real bus bridge magically knows whether a > >>>>>>>>>>>>>> target > >>>>>>>>>>>>>> address maps to RAM or not. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> What I think is really going on is that even for systems > >>>>>>>>>>>>>> without an > >>>>>>>>>>>>>> IOMMU, it's not really true to say that the PCI address space > >>>>>>>>>>>>>> maps > >>>>>>>>>>>>>> directly onto address_space_memory. Instead, there's a large, > >>>>>>>>>>>>>> but > >>>>>>>>>>>>>> much less than 2^64 sized, "upstream window" at address 0 on > >>>>>>>>>>>>>> the PCI > >>>>>>>>>>>>>> bus, which is identity mapped to the system bus. Details will > >>>>>>>>>>>>>> vary > >>>>>>>>>>>>>> with the system, but in practice we expect nothing but RAM to > >>>>>>>>>>>>>> be in > >>>>>>>>>>>>>> that window. Addresses not within that window won't be mapped > >>>>>>>>>>>>>> to the > >>>>>>>>>>>>>> system bus but will just be broadcast on the PCI bus and might > >>>>>>>>>>>>>> be > >>>>>>>>>>>>>> picked up as a p2p transaction. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Currently this p2p works only via the IOMMU, direct p2p is not > >>>>>>>>>>>>> possible as > >>>>>>>>>>>>> the guest needs to know physical MMIO addresses to make p2p > >>>>>>>>>>>>> work and it > >>>>>>>>>>>>> does not. > >>>>>>>>>>>> > >>>>>>>>>>>> /me points to the Direct Translated P2P section of the ACS spec, > >>>>>>>>>>>> though > >>>>>>>>>>>> it's as prone to spoofing by the device as ATS. In any case, p2p > >>>>>>>>>>>> reflected from the IOMMU is still p2p and offloads the CPU even > >>>>>>>>>>>> if > >>>>>>>>>>>> bandwidth suffers vs bare metal depending on if the data doubles > >>>>>>>>>>>> back > >>>>>>>>>>>> over any links. Thanks, > >>>>>>>>>>> > >>>>>>>>>>> Sure, I was just saying that p2p via IOMMU won't be as simple as > >>>>>>>>>>> broadcast > >>>>>>>>>>> on the PCI bus, IOMMU needs to be programmed in advance to make > >>>>>>>>>>> this work, > >>>>>>>>>>> and current that broadcast won't work for the passed through > >>>>>>>>>>> devices. > >>>>>>>>>> > >>>>>>>>>> Well, sure, p2p in a guest with passthrough devices clearly needs > >>>>>>>>>> to > >>>>>>>>>> be translated through the IOMMU (and p2p from a passthrough to an > >>>>>>>>>> emulated device is essentially impossible). > >>>>>>>>>> > >>>>>>>>>> But.. what does that have to do with this code. This is the memory > >>>>>>>>>> area watcher, looking for memory regions being mapped directly into > >>>>>>>>>> the PCI space. NOT IOMMU regions, since those are handled > >>>>>>>>>> separately > >>>>>>>>>> by wiring up the IOMMU notifier. This will only trigger if > >>>>>>>>>> RAM-like, > >>>>>>>>>> non-RAM regions are put into PCI space *not* behind an IOMMMU. > >>>>>>>>> > >>>>>>>>> Duh, sorry, realised I was mixing up host and guest IOMMU. I guess > >>>>>>>>> the point here is that this will map RAM-like devices into the host > >>>>>>>>> IOMMU when there is no guest IOMMU, allowing p2p transactions > >>>>>>>>> between > >>>>>>>>> passthrough devices (though not from passthrough to emulated > >>>>>>>>> devices). > >>>>>>>> > >>>>>>>> Correct. > >>>>>>>> > >>>>>>>>> > >>>>>>>>> The conditions still seem kind of awkward to me, but I guess it > >>>>>>>>> makes > >>>>>>>>> sense. > >>>>>>>> > >>>>>>>> Is it the time to split this listener to RAM-listener and PCI bus > >>>>>>>> listener? > >>>>>>> > >>>>>>> I'm not really sure what you mean by that. > >>>>>>> > >>>>>>>> On x86 it listens on the "memory" AS, on spapr - on the > >>>>>>>> "pci@800000020000000" AS, this will just create more confusion over > >>>>>>>> time... > >>>>>>> > >>>>>>> Hm, it's still logically the same AS in each case - the PCI address > >>>>>>> space. It's just that in the x86 case that happens to be the same as > >>>>>>> the memory AS (at least when there isn't a guest side IOMMU). > >>>>>> > >>>>>> Hm. Ok. > >>>>>> > >>>>>>> I do wonder if that's really right even for x86 without IOMMU. The > >>>>>>> PCI address space is identity mapped to the "main" memory address > >>>>>>> space there, but I'm not sure it's mapped th *all* of the main memory > >>>>>>> > >>>>>> > >>>>>> What should have been in the place of "th" above? :) > >>>>>> > >>>>>>> address space, probably just certain ranges of it. That's what I was > >>>>>>> suggesting earlier in the thread. > >>>>>> > >>>>>> afaict vfio is trying to mmap every RAM MR == KVM memory slot, no > >>>>>> ranges or > >>>>>> anything like that. I am trying to figure out what is not correct with > >>>>>> the > >>>>>> patch. Thanks, > >>>>> > >>>>> It is possible for x86 systems to have translation applied to MMIO vs > >>>>> RAM such that the processor view and device view of MMIO are different, > >>>>> putting them into separate address spaces, but this is not typical and > >>>>> not available on the class of chipsets that QEMU emulates for PC. > >>>>> Therefore I think it's correct that MMIO and RAM all live in one big > >>>>> happy, flat address space as they do now (assuming the guest IOMMU is > >>>>> not present or disabled). Thanks, > >>>> > >>>> Ah.. I think the thing I was missing is that on PC (at least with > >>>> typical chipsets) *all* MMIO essentially comes from PCI space. Even > >>>> the legacy devices are essentially ISA which is treated as being on a > >>>> bridge under the PCI space. On non-x86 there are often at least a > >>>> handful of MMIO devices that aren't within the PCI space - say, the > >>>> PCI host bridge itself at least. x86 avoids that by using the > >>>> separate IO space, which is also a bit weird in that it's > >>>> simultaneously direct attached to the cpu (and so doesn't need bridge > >>>> configuration), but also identified with the legay IO space treated as > >>>> being under two bridges (host->PCI, PCI->ISA) for other purposes. > >>>> > >>>> Maybe it's just me, but I find it makes more sense to me if I think of > >>>> it as the two ASes being equal because on PC the system address map > >>>> (address_space_memory) is made equal to the PCI address map, rather > >>>> than the PCI address map being made equal to the system one. > >>> > >>> It makes more sense to me too, we just do not exploit/expose this on SPAPR > >>> - the PCI address space only has 2xIOMMU and 1xMSI windows and that's it, > >>> no MMIO which is mapped to the system AS only (adding those to the PCI AS > >>> is little tricky as mentioned elsewhere). > >>> > >>> Anyway, I still wonder - what needs to be addressed in the 2/4 patch in > >>> order to proceed? > >> > >> Ping? > >> > > > > > > Ping? > > > > > > > Could anyone please enlighten me what needs to be done with this patchset > to proceed, or confirm that it is ok but not going anywhere as everybody is > busy with other things? :) Thanks, Actually making sure it compiles would have been nice: qemu.git/hw/vfio/common.c: In function ‘vfio_listener_region_add’: qemu.git/hw/vfio/common.c:550:40: error: invalid operands to binary & (have ‘Int128 {aka struct Int128}’ and ‘hwaddr {aka long long unsigned int}’) if ((iova & pgmask) || (llsize & pgmask)) { ^ qemu.git/hw/vfio/common.c: In function ‘vfio_listener_region_del’: qemu.git/hw/vfio/common.c:669:50: error: invalid operands to binary & (have ‘Int128 {aka struct Int128}’ and ‘hwaddr {aka long long unsigned int}’) try_unmap = !((iova & pgmask) || (llsize & pgmask)); ^ Clearly llsize needs to be wrapped in int128_get64() here. Should I presume that testing of this patch is equally lacking? Eric, have you done any testing of this? I think this was fixing a mapping issue you reported. Thanks, Alex