On Wed, Mar 23, 2016 at 02:06:36PM +1100, Alexey Kardashevskiy wrote: > On 03/23/2016 01:53 PM, David Gibson wrote: > >On Wed, Mar 23, 2016 at 01:12:59PM +1100, Alexey Kardashevskiy wrote: > >>On 03/23/2016 12:08 PM, David Gibson wrote: > >>>On Tue, Mar 22, 2016 at 04:54:07PM +1100, Alexey Kardashevskiy wrote: > >>>>On 03/22/2016 04:14 PM, David Gibson wrote: > >>>>>On Mon, Mar 21, 2016 at 06:47:05PM +1100, Alexey Kardashevskiy wrote: > >>>>>>New VFIO_SPAPR_TCE_v2_IOMMU type supports dynamic DMA window management. > >>>>>>This adds ability to VFIO common code to dynamically allocate/remove > >>>>>>DMA windows in the host kernel when new VFIO container is added/removed. > >>>>>> > >>>>>>This adds VFIO_IOMMU_SPAPR_TCE_CREATE ioctl to vfio_listener_region_add > >>>>>>and adds just created IOMMU into the host IOMMU list; the opposite > >>>>>>action is taken in vfio_listener_region_del. > >>>>>> > >>>>>>When creating a new window, this uses euristic to decide on the TCE > >>>>>>table > >>>>>>levels number. > >>>>>> > >>>>>>This should cause no guest visible change in behavior. > >>>>>> > >>>>>>Signed-off-by: Alexey Kardashevskiy <[email protected]> > >>>>>>--- > >>>>>>Changes: > >>>>>>v14: > >>>>>>* new to the series > >>>>>> > >>>>>>--- > >>>>>>TODO: > >>>>>>* export levels to PHB > >>>>>>--- > >>>>>> hw/vfio/common.c | 108 > >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++--- > >>>>>> trace-events | 2 ++ > >>>>>> 2 files changed, 105 insertions(+), 5 deletions(-) > >>>>>> > >>>>>>diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >>>>>>index 4e873b7..421d6eb 100644 > >>>>>>--- a/hw/vfio/common.c > >>>>>>+++ b/hw/vfio/common.c > >>>>>>@@ -279,6 +279,14 @@ static int vfio_host_iommu_add(VFIOContainer > >>>>>>*container, > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>>+static void vfio_host_iommu_del(VFIOContainer *container, hwaddr > >>>>>>min_iova) > >>>>>>+{ > >>>>>>+ VFIOHostIOMMU *hiommu = vfio_host_iommu_lookup(container, > >>>>>>min_iova, 0x1000); > >>>>> > >>>>>The hard-coded 0x1000 looks dubious.. > >>>> > >>>>Well, that's the minimal page size... > >>> > >>>Really? Some BookE CPUs support 1KiB page size.. > >> > >>Hm. For IOMMU? Ok. s/0x1000/1/ should do then :) > > > >Uh.. actually I don't think those CPUs generally had an IOMMU. But if > >it's been done for CPU MMU I wouldn't count on it not being done for > >IOMMU. > > > >1 is a safer choice. > > > >> > >> > >>> > >>>>>>+ g_assert(hiommu); > >>>>>>+ QLIST_REMOVE(hiommu, hiommu_next); > >>>>>>+} > >>>>>>+ > >>>>>> static bool vfio_listener_skipped_section(MemoryRegionSection > >>>>>> *section) > >>>>>> { > >>>>>> return (!memory_region_is_ram(section->mr) && > >>>>>>@@ -392,6 +400,61 @@ static void > >>>>>>vfio_listener_region_add(MemoryListener *listener, > >>>>>> } > >>>>>> end = int128_get64(llend); > >>>>>> > >>>>>>+ if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) { > >>>>> > >>>>>I think this would be clearer split out into a helper function, > >>>>>vfio_create_host_window() or something. > >>>> > >>>> > >>>>It is rather vfio_spapr_create_host_window() and we were avoiding > >>>>xxx_spapr_xxx so far. I'd cut-n-paste the SPAPR PCI AS listener to a > >>>>separate file but this usually triggers more discussion and never ends > >>>>well. > >>>> > >>>> > >>>> > >>>>>>+ unsigned entries, pages; > >>>>>>+ struct vfio_iommu_spapr_tce_create create = { .argsz = > >>>>>>sizeof(create) }; > >>>>>>+ > >>>>>>+ g_assert(section->mr->iommu_ops); > >>>>>>+ g_assert(memory_region_is_iommu(section->mr)); > >>>>> > >>>>>I don't think you need these asserts. AFAICT the same logic should > >>>>>work if a RAM MR was added directly to PCI address space - this would > >>>>>create the new host window, then the existing code for adding a RAM MR > >>>>>would map that block of RAM statically into the new window. > >>>> > >>>>In what configuration/machine can we do that on SPAPR? > >>> > >>>spapr guests won't ever do that. But you can run an x86 guest on a > >>>powernv host and this situation could come up. > >> > >> > >>I am pretty sure VFIO won't work in this case anyway. > > > >I'm not. There's no fundamental reason VFIO shouldn't work with TCG. > > This is not about TCG (pseries TCG guest works with VFIO on powernv host), > this is about things like VFIO_IOMMU_GET_INFO vs. > VFIO_IOMMU_SPAPR_TCE_GET_INFO ioctls but yes, fundamentally, it can work. > > Should I add such support in this patchset?
Unless adding the generality is really complex, and so far I haven't
seen a reason for it to be.
>
>
> >
> >>>In any case there's no point asserting if the code is correct anyway.
> >>
> >>Assert here says (at least) "not tested" or "not expected to
> >>happen".
> >
> >Hmmm..
> >
> >>
> >>
> >>>
> >>>>>>+ trace_vfio_listener_region_add_iommu(iova, end - 1);
> >>>>>>+ /*
> >>>>>>+ * FIXME: For VFIO iommu types which have KVM acceleration to
> >>>>>>+ * avoid bouncing all map/unmaps through qemu this way, this
> >>>>>>+ * would be the right place to wire that up (tell the KVM
> >>>>>>+ * device emulation the VFIO iommu handles to use).
> >>>>>>+ */
> >>>>>>+ create.window_size = memory_region_size(section->mr);
> >>>>>>+ create.page_shift =
> >>>>>>+
> >>>>>>ctz64(section->mr->iommu_ops->get_page_sizes(section->mr));
> >>>>>
> >>>>>Ah.. except that I guess you'd need to fall back to host page size
> >>>>>here to handle a RAM MR.
> >>>>
> >>>>Can you give an example of such RAM MR being added to PCI AS on
> >>>>SPAPR?
> >>>
> >>>On spapr, no. But you can run other machine types as guests (at least
> >>>with TCG) on a host with the spapr IOMMU.
> >>>
> >>>>>>+ /*
> >>>>>>+ * SPAPR host supports multilevel TCE tables, there is some
> >>>>>>+ * euristic to decide how many levels we want for our table:
> >>>>>>+ * 0..64 = 1; 65..4096 = 2; 4097..262144 = 3; 262145.. = 4
> >>>>>>+ */
> >>>>>>+ entries = create.window_size >> create.page_shift;
> >>>>>>+ pages = (entries * sizeof(uint64_t)) / getpagesize();
> >>>>>>+ create.levels = ctz64(pow2ceil(pages) - 1) / 6 + 1;
> >>>>>>+
> >>>>>>+ ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_CREATE,
> >>>>>>&create);
> >>>>>>+ if (ret) {
> >>>>>>+ error_report("Failed to create a window, ret = %d (%m)",
> >>>>>>ret);
> >>>>>>+ goto fail;
> >>>>>>+ }
> >>>>>>+
> >>>>>>+ if (create.start_addr != section->offset_within_address_space
> >>>>>>||
> >>>>>>+ vfio_host_iommu_lookup(container, create.start_addr,
> >>>>>>+ create.start_addr +
> >>>>>>create.window_size - 1)) {
> >>>>>
> >>>>>Under what circumstances can this trigger? Is the kernel ioctl
> >>>>>allowed to return a different window start address than the one
> >>>>>requested?
> >>>>
> >>>>You already asked this some time ago :) The userspace cannot request
> >>>>address, the host kernel returns one.
> >>>
> >>>Ok. For generality it would be nice if you could succeed here as long
> >>>as the new host window covers the requested guest window, even if it
> >>>doesn't match exactly. And for that matter to not request the new
> >>>window if the host already has a window covering the guest region.
> >>
> >>
> >>That would be dead code - when would it possibly work? I mean I could
> >>instrument an artificial test but the actual user which might appear later
> >>will likely be soooo different so this won't help anyway.
> >
> >Hmm, I suppose. It actually shouldn't be that hard to trigger a case
> >like this, if you just bumped the bridge's dma64 base address property
> >up a little bit - above the host kernel's base address, but small
> >enough that you can still easily fit the guest memory in.
>
>
> I can test it today for sure but once committed, we will have to support it.
> Which I am trying to avoid until we get clear picture what we are supporting
> here.
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
