[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU |
Date: |
Wed, 23 Mar 2016 13:53:16 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> >>>>---
> >>>>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.
> >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.
> >>>The second check looks very strange - if it returns true doesn't that
> >>>mean you *do* have host window which can accomodate this guest region,
> >>>which is what you want?
> >>
> >>This should not happen, this is what this check is for. Can make it assert()
> >>or something like this.
> >
> >Oh.. I see. Because you've done the ioctl, but not recorded the new
> >host window in the list yet.
> >
> >No, I think the correct approach is to look for an existing host
> >window containing the requested guest window *before* you try to
> >create a new host window. If one is already there, you can just carry
> >on.
>
> Right, I'll change this.
>
>
> >
> >>>>+ struct vfio_iommu_spapr_tce_remove remove = {
> >>>>+ .argsz = sizeof(remove),
> >>>>+ .start_addr = create.start_addr
> >>>>+ };
> >>>>+ error_report("Host doesn't support DMA window at
> >>>>%"HWADDR_PRIx", must be %"PRIx64,
> >>>>+ section->offset_within_address_space,
> >>>>+ create.start_addr);
> >>>>+ ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_REMOVE, &remove);
> >>>>+ ret = -EINVAL;
> >>>>+ goto fail;
> >>>>+ }
> >>>>+ trace_vfio_spapr_create_window(create.page_shift,
> >>>>+ create.window_size,
> >>>>+ create.start_addr);
> >>>>+
> >>>>+ vfio_host_iommu_add(container, create.start_addr,
> >>>>+ create.start_addr + create.window_size - 1,
> >>>>+ 1ULL << create.page_shift);
> >>>>+ }
> >>>>+
> >>>> if (!vfio_host_iommu_lookup(container, iova, end - 1)) {
> >>>> error_report("vfio: IOMMU container %p can't map guest IOVA
> >>>> region"
> >>>> " 0x%"HWADDR_PRIx"..0x%"HWADDR_PRIx,
> >>>>@@ -525,6 +588,22 @@ static void vfio_listener_region_del(MemoryListener
> >>>>*listener,
> >>>> container, iova, end - iova, ret);
> >>>> }
> >>>>
> >>>>+ if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> >>>>+ struct vfio_iommu_spapr_tce_remove remove = {
> >>>>+ .argsz = sizeof(remove),
> >>>>+ .start_addr = section->offset_within_address_space,
> >>>>+ };
> >>>>+ ret = ioctl(container->fd, VFIO_IOMMU_SPAPR_TCE_REMOVE, &remove);
> >>>>+ if (ret) {
> >>>>+ error_report("Failed to remove window at %"PRIx64,
> >>>>+ remove.start_addr);
> >>>>+ }
> >>>>+
> >>>>+ vfio_host_iommu_del(container,
> >>>>section->offset_within_address_space);
> >>>>+
> >>>>+ trace_vfio_spapr_remove_window(remove.start_addr);
> >>>>+ }
> >>>>+
> >>>> if (iommu && iommu->iommu_ops && iommu->iommu_ops->vfio_stop) {
> >>>> iommu->iommu_ops->vfio_stop(section->mr);
> >>>> }
> >>>>@@ -928,11 +1007,30 @@ static int vfio_connect_container(VFIOGroup
> >>>>*group, AddressSpace *as)
> >>>> goto listener_release_exit;
> >>>> }
> >>>>
> >>>>- /* The default table uses 4K pages */
> >>>>- vfio_host_iommu_add(container, info.dma32_window_start,
> >>>>- info.dma32_window_start +
> >>>>- info.dma32_window_size - 1,
> >>>>- 0x1000);
> >>>>+ if (v2) {
> >>>>+ /*
> >>>>+ * There is a default window in just created container.
> >>>>+ * To make region_add/del simpler, we better remove this
> >>>>+ * window now and let those iommu_listener callbacks
> >>>>+ * create/remove them when needed.
> >>>>+ */
> >>>>+ struct vfio_iommu_spapr_tce_remove remove = {
> >>>>+ .argsz = sizeof(remove),
> >>>>+ .start_addr = info.dma32_window_start,
> >>>>+ };
> >>>>+ ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_REMOVE, &remove);
> >>>>+ if (ret) {
> >>>>+ error_report("vfio: VFIO_IOMMU_SPAPR_TCE_REMOVE failed:
> >>>>%m");
> >>>>+ ret = -errno;
> >>>>+ goto free_container_exit;
> >>>>+ }
> >>>>+ } else {
> >>>>+ /* The default table uses 4K pages */
> >>>>+ vfio_host_iommu_add(container, info.dma32_window_start,
> >>>>+ info.dma32_window_start +
> >>>>+ info.dma32_window_size - 1,
> >>>>+ 0x1000);
> >>>>+ }
> >>>> } else {
> >>>> error_report("vfio: No available IOMMU models");
> >>>> ret = -EINVAL;
> >>>>diff --git a/trace-events b/trace-events
> >>>>index cc619e1..f2b75a3 100644
> >>>>--- a/trace-events
> >>>>+++ b/trace-events
> >>>>@@ -1736,6 +1736,8 @@ vfio_region_finalize(const char *name, int index)
> >>>>"Device %s, region %d"
> >>>> vfio_region_mmaps_set_enabled(const char *name, bool enabled) "Region
> >>>> %s mmaps enabled: %d"
> >>>> vfio_ram_register(uint64_t va, uint64_t size, int ret) "va=%"PRIx64"
> >>>> size=%"PRIx64" ret=%d"
> >>>> vfio_ram_unregister(uint64_t va, uint64_t size, int ret) "va=%"PRIx64"
> >>>> size=%"PRIx64" ret=%d"
> >>>>+vfio_spapr_create_window(int ps, uint64_t ws, uint64_t off)
> >>>>"pageshift=0x%x winsize=0x%"PRIx64" offset=0x%"PRIx64
> >>>>+vfio_spapr_remove_window(uint64_t off) "offset=%"PRIx64
> >>>>
> >>>> # hw/vfio/platform.c
> >>>> vfio_platform_base_device_init(char *name, int groupid) "%s belongs to
> >>>> group #%d"
> >>>
> >>
> >>
> >
>
>
--
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
- Re: [Qemu-ppc] [PATCH qemu v14 08/18] spapr_iommu: Migrate full state, (continued)
- [Qemu-ppc] [PATCH qemu v14 14/18] spapr_pci: Add and export DMA resetting helper, Alexey Kardashevskiy, 2016/03/21
- [Qemu-ppc] [PATCH qemu v14 15/18] vfio: Add host side IOMMU capabilities, Alexey Kardashevskiy, 2016/03/21
- [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/21
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, David Gibson, 2016/03/22
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/22
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, David Gibson, 2016/03/22
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/22
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU,
David Gibson <=
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/22
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, David Gibson, 2016/03/23
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/23
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/24
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, David Gibson, 2016/03/29
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, Alexey Kardashevskiy, 2016/03/29
- Re: [Qemu-ppc] [PATCH qemu v14 17/18] vfio/spapr: Use VFIO_SPAPR_TCE_v2_IOMMU, David Gibson, 2016/03/29
[Qemu-ppc] [PATCH qemu v14 13/18] vfio: spapr: Add SPAPR IOMMU v2 support (DMA memory preregistering), Alexey Kardashevskiy, 2016/03/21
[Qemu-ppc] [PATCH qemu v14 11/18] memory: Add reporting of supported page sizes, Alexey Kardashevskiy, 2016/03/21