vfio: Align iova also to IOMMU page size

On some architectures TARGET_PAGE_ALIGN() is not enough to get the right alignment. For example on ARM TARGET_PAGE_BITS is 10 because some old CPUs support 1K page size, while minimum SMMU page size is 4K.

This fixes problems like:

2015-11-17T07:37:42.892265Z qemu-system-aarch64: VFIO_MAP_DMA: -22
2015-11-17T07:37:42.892309Z qemu-system-aarch64: vfio_dma_map(0x223da230, 0x80002f0400, 0x10fc00, 0x7f89b40400) = -22 (Invalid

argument)
qemu: hardware error: vfio: DMA mapping failed, unable to continue

Signed-off-by Pavel Fedin <[email protected]>

---
 hw/vfio/common.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

hw/vfio/common.c +3 -1

index ff5a89a..328140c 100644
@@ -326,7 +326,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
                                      MemoryRegionSection *section)
 {
     VFIOContainer *container = container_of(listener, VFIOContainer, listener);
-    hwaddr iova, end;
+    hwaddr iova, end, iommu_page_size;
     Int128 llend;
     void *vaddr;
     int ret;

@@ -346,6 +346,8 @@ static void vfio_listener_region_add(MemoryListener *listener,
     }
 
     iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
+    iommu_page_size = vfio_container_granularity(container);
+    iova = (iova + iommu_page_size - 1) & ~(iommu_page_size - 1);
     llend = int128_make64(section->offset_within_address_space);
     llend = int128_add(llend, section->size);
     llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));

Quoted 8 lines

On some architectures TARGET_PAGE_ALIGN() is not enough to get the right alignment. For example on ARM TARGET_PAGE_BITS is 10 because some old CPUs support 1K page size, while minimum SMMU page size is 4K.

This fixes problems like:

2015-11-17T07:37:42.892265Z qemu-system-aarch64: VFIO_MAP_DMA: -22
2015-11-17T07:37:42.892309Z qemu-system-aarch64: vfio_dma_map(0x223da230, 0x80002f0400,

0x10fc00, 0x7f89b40400) = -22 (Invalid

Quoted 2 lines

argument)
qemu: hardware error: vfio: DMA mapping failed, unable to continue

I don't understand how this is supposed to work, if we align to a larger size than the processor, then there are processor size pages of RAM than could be handed out as DMA targets for devices, but we can't map them through the IOMMU. Thus if the guest tries to use them, we get IOMMU faults in the host and likely memory corruption in the guest because the device can't read or write to the page it's supposed to. This doesn't seem like the right solution.

Hello!

Quoted 11 lines

Quoted 8 lines

On some architectures TARGET_PAGE_ALIGN() is not enough to get the right alignment. For example on ARM TARGET_PAGE_BITS is 10 because some old CPUs support 1K page size, while minimum SMMU page size is 4K.

This fixes problems like:

2015-11-17T07:37:42.892265Z qemu-system-aarch64: VFIO_MAP_DMA: -22
2015-11-17T07:37:42.892309Z qemu-system-aarch64: vfio_dma_map(0x223da230, 0x80002f0400,

0x10fc00, 0x7f89b40400) = -22 (Invalid

Quoted 2 lines

argument)
qemu: hardware error: vfio: DMA mapping failed, unable to continue

[skip]

Quoted 7 lines

I don't understand how this is supposed to work, if we align to a larger size than the processor, then there are processor size pages of RAM than could be handed out as DMA targets for devices, but we can't map them through the IOMMU. Thus if the guest tries to use them, we get IOMMU faults in the host and likely memory corruption in the guest because the device can't read or write to the page it's supposed to. This doesn't seem like the right solution.

Well, this was my first try on the problem. I've got your idea. But i guess we should discuss the proper solution then.
So, i've got this problem on ARM64. On ARM64 we actually can never have 1K pages. This page size was supported only by old 32-bit ARM CPUs, up to ARMv5 IIRC, then it was dropped. Linux OS never even used it. But, since qemu can emulate those ancient CPUs, TARGET_PAGE_BITS is defined to 10 for ARM. And, ARM64 and ARM32 is actually the same target for qemu, so this is why we still get it. Perhaps, TARGET_PAGE_BITS should be a variable for ARM, and we should set it according to the actual used CPU. Then this IOMMU alignment problem would disappear automatically. What do you think? Cc'ed Peter since he is the main ARM guy here.

Quoted 9 lines

So, i've got this problem on ARM64. On ARM64 we actually can never have 1K pages. This page
size was supported only by old 32-bit ARM CPUs, up to ARMv5 IIRC, then it was dropped. Linux OS never even used it.
But, since qemu can emulate those ancient CPUs, TARGET_PAGE_BITS is defined to 10 for ARM.
And, ARM64 and ARM32 is actually the same target for qemu, so this is why we still get it.
Perhaps, TARGET_PAGE_BITS should be a variable for ARM, and we should set it according to
the actual used CPU. Then this IOMMU alignment problem would disappear automatically. What do you think?
Cc'ed Peter since he is the main ARM guy here.

Do we only see these alignments when we're emulating those old 1k page processors?

If not, should we really be telling a 4k page VM about 1k aligned memory?

If that's all legit, maybe we should be aligning down
rather than up, we know the host memory is at least 4k pages, so anything in the gap between those alignments should be backed by memory, right?

Hello!

Quoted 10 lines

Quoted 9 lines

So, i've got this problem on ARM64. On ARM64 we actually can never have 1K pages. This page
size was supported only by old 32-bit ARM CPUs, up to ARMv5 IIRC, then it was dropped. Linux OS never even used it.
But, since qemu can emulate those ancient CPUs, TARGET_PAGE_BITS is defined to 10 for ARM.
And, ARM64 and ARM32 is actually the same target for qemu, so this is why we still get it.
Perhaps, TARGET_PAGE_BITS should be a variable for ARM, and we should set it according to
the actual used CPU. Then this IOMMU alignment problem would disappear automatically. What do you think?
Cc'ed Peter since he is the main ARM guy here.

Quoted 2 lines

Do we only see these alignments when we're emulating those old 1k page processors?

No. We see them when we are emulating brand new ARM64. And we see them only because TARGET_PAGE_BITS is still hardcoded to 10.

Quoted 2 lines

If not, should we really be telling a 4k page VM about 1k aligned memory?

Well, first of all, we are not telling about aligned memory at all. I've researched for the origin of this address, it appears when we are mapping MSI-X BAR of the VFIO device.
My device defines this BAR to be of 2M size. In this case qemu splits it up into three regions:
1) Region below the MSI-X table (it's called "mmap", for me it's empty because table offset is 0)
2) MSI-X table itself (20 vectors = 0x00000140 bytes for me).
3) Region above the MSi-X table (it's called "mmap msix-hi"). Size for my case is 2M - REAL_HOST_PAGE_ALIGN(0x140) = 0x1FF000.
Regions (1) and (3) are passed through and directly mapped by VFIO, region (2) is emulated.
Now, we have PBA. The device says that PBA is placed in memory specified by the same BAR as table, and its offset is 0x000f0000. PBA is also emulated by qemu, and as a result it overlaps "mmap msix-hi" region, which breaks up into two fragments as a consequence:
3a) offset 0x00000 size 0x0EF000
3b) offset 0xEF008 size 0x10FFF8
PBA sits between these fragments, having only 8 bytes in size.
Attempt to map (3b) causes the problem. vfio_mmap_region() is expected to align this up, and it does, but it does it to a minimum possible granularity for ARM platform, which, as qemu thinks, is always 1K.

Quoted 9 lines

My device defines this BAR to be of 2M size. In this case qemu splits it up into three
regions:
1) Region below the MSI-X table (it's called "mmap", for me it's empty because table offset
is 0)
2) MSI-X table itself (20 vectors = 0x00000140 bytes for me).
3) Region above the MSi-X table (it's called "mmap msix-hi"). Size for my case is 2M -
REAL_HOST_PAGE_ALIGN(0x140) = 0x1FF000.
Regions (1) and (3) are passed through and directly mapped by VFIO, region (2) is emulated.
Now, we have PBA. The device says that PBA is placed in memory specified by the same BAR as

Quoted 8 lines

table, and its offset is 0x000f0000. PBA is also emulated by qemu, and as a result it overlaps "mmap msix-hi" region, which breaks up into two fragments as a consequence:
3a) offset 0x00000 size 0x0EF000
3b) offset 0xEF008 size 0x10FFF8
PBA sits between these fragments, having only 8 bytes in size.
Attempt to map (3b) causes the problem. vfio_mmap_region() is expected to align this up,
and it does, but it does it to a minimum possible granularity for ARM platform, which, as qemu thinks, is always 1K.

Yep, on x86 we'd have target page size == host page size == minimum iommu page size and we'd align that up to something that can be mapped, which means we wouldn't have an iommu mapping for peer-to-peer in this space, but nobody is likely to notice.

Hello!

Quoted 18 lines

Quoted 9 lines

My device defines this BAR to be of 2M size. In this case qemu splits it up into three
regions:
1) Region below the MSI-X table (it's called "mmap", for me it's empty because table offset
is 0)
2) MSI-X table itself (20 vectors = 0x00000140 bytes for me).
3) Region above the MSi-X table (it's called "mmap msix-hi"). Size for my case is 2M -
REAL_HOST_PAGE_ALIGN(0x140) = 0x1FF000.
Regions (1) and (3) are passed through and directly mapped by VFIO, region (2) is emulated.
Now, we have PBA. The device says that PBA is placed in memory specified by the same BAR as

Quoted 8 lines

table, and its offset is 0x000f0000. PBA is also emulated by qemu, and as a result it overlaps "mmap msix-hi" region, which breaks up into two fragments as a consequence:
3a) offset 0x00000 size 0x0EF000
3b) offset 0xEF008 size 0x10FFF8
PBA sits between these fragments, having only 8 bytes in size.
Attempt to map (3b) causes the problem. vfio_mmap_region() is expected to align this up,
and it does, but it does it to a minimum possible granularity for ARM platform, which, as qemu thinks, is always 1K.

Quoted 4 lines

Yep, on x86 we'd have target page size == host page size == minimum iommu page size and we'd align that up to something that can be mapped, which means we wouldn't have an iommu mapping for peer-to-peer in this space, but nobody is likely to notice.

So, OK, to keep the quoting short... I've also read your previous reply about that "rounding up just works". Let's sum things up.

1. Rounding up "just works" in my case too. So i don't see a direct reason to change it.
2. The only problem is rounding up to 1K, which TARGET_PAGE_ALIGN() does on ARM. What we need is 4K, which is appropriate for host too. And, by the way, for modern ARM guests too. So, we can do either of:
a) Keep my original approach - TARGET_PAGE_ALIGN(), then align to vfio_container_granularity().
b) Just align to vfio_container_granularity() instead of using TARGET_PAGE_ALIGN().
c) Use HOST_PAGE_ALIGN() instead of TARGET_PAGE_ALIGN() (what Peter suggested).

On x86 there would be no difference, because all these alignments are identical. On ARM, actually, all of these approaches would also give correct result, because it's only TARGET_PAGE_ALIGN() wrong in this case. So - what do you think is the most appropriate? Let's make a choice and i'll send a proper patch.

I feel a lot more comfortable if we limit the scope to MMIO regions of PCI devices. The problems I brought up before about the device not being able to DMA to a target aligned RAM address are still a possibility that I think we want to catch. To do that, I think we just need:

Object *obj = memory_region_owner(section->mr);

if (object_dynamic_cast(obj, "pci-device")) {
    /* HOST_PAGE_ALIGN... */
} else {
    /* TARGET_PAGE_ALIGN... */
}

Quoted 8 lines · Alex Williamson

I feel a lot more comfortable if we limit the scope to MMIO regions of PCI devices. The problems I brought up before about the device not being able to DMA to a target aligned RAM address are still a possibility that I think we want to catch. To do that, I think we just need:

Object *obj = memory_region_owner(section->mr);

Quoted 5 lines

if (object_dynamic_cast(obj, "pci-device")) {
    /* HOST_PAGE_ALIGN... */
} else {
    /* TARGET_PAGE_ALIGN... */
}

This looks very odd to me, in two ways: (a) behaving differently for PCI passthrough vs other kinds of passthrough,

and (b) caring
about TARGET_PAGE_ALIGN at all. TARGET_PAGE_ALIGN really isn't something vfio should need to care about I think.

Quoted 15 lines · Peter Maydell

Quoted 13 lines · Alex Williamson

I feel a lot more comfortable if we limit the scope to MMIO regions of PCI devices. The problems I brought up before about the device not being able to DMA to a target aligned RAM address are still a possibility that I think we want to catch. To do that, I think we just need:

Object *obj = memory_region_owner(section->mr);

if (object_dynamic_cast(obj, "pci-device")) {
    /* HOST_PAGE_ALIGN... */
} else {
    /* TARGET_PAGE_ALIGN... */
}

Quoted 2 lines

This looks very odd to me, in two ways: (a) behaving differently for PCI passthrough vs other kinds of passthrough,

It's a matter of risk. If we align an MMIO range out of existence all we've prevented is peer-to-peer DMA between assigned devices. Chances of anyone caring about that are slim to none. If we align RAM out of existence, that's a much, much more significant risk that we've just introduced a data integrity issue for the VM.

Quoted 3 lines

and (b) caring
about TARGET_PAGE_ALIGN at all. TARGET_PAGE_ALIGN really isn't something vfio should need to care about I think.

But I think we do. If a RAM address is target page aligned, it could be a valid DMA target for the device.

If we align it out of existence and
the device is programmed to perform a DMA to that address, the IOMMU will block it, the VM will not be informed and will continue executing with invalid data.

Quoted 16 lines · Alex Williamson

Quoted 15 lines · Peter Maydell

Quoted 13 lines · Alex Williamson

I feel a lot more comfortable if we limit the scope to MMIO regions of PCI devices. The problems I brought up before about the device not being able to DMA to a target aligned RAM address are still a possibility that I think we want to catch. To do that, I think we just need:

Object *obj = memory_region_owner(section->mr);

if (object_dynamic_cast(obj, "pci-device")) {
    /* HOST_PAGE_ALIGN... */
} else {
    /* TARGET_PAGE_ALIGN... */
}

Quoted 8 lines

Quoted 2 lines

This looks very odd to me, in two ways: (a) behaving differently for PCI passthrough vs other kinds of passthrough,

It's a matter of risk. If we align an MMIO range out of existence all we've prevented is peer-to-peer DMA between assigned devices. Chances of anyone caring about that are slim to none. If we align RAM out of existence, that's a much, much more significant risk that we've just introduced a data integrity issue for the VM.

I don't see why this is different for PCI devices versus memory-mapped passthrough devices, though. If what you mean is "is this MemoryRegion not RAM" maybe you want if (!memory_region_is_ram(mr)) ?

Quoted 4 lines

Quoted 3 lines

and (b) caring
about TARGET_PAGE_ALIGN at all. TARGET_PAGE_ALIGN really isn't something vfio should need to care about I think.

Quoted 2 lines

But I think we do. If a RAM address is target page aligned, it could be a valid DMA target for the device.

TARGET_PAGE_ALIGN doesn't tell you whether an address is actually page aligned for the guest, though. In fact, you can't tell what page size the guest happens to be using (or what the alignment restrictions on doing DMA might be, or the page size being used by the IOMMU, which isn't necessarily the guest page size either).

Quoted 4 lines

If we align it out of existence and
the device is programmed to perform a DMA to that address, the IOMMU will block it, the VM will not be informed and will continue executing with invalid data.

Shouldn't this cause the device to say "hey, my DMA transaction failed, I will flag that up as an error" ? (That's not much better as a failure situation, of course.)

TAGET_PAGE_ALIGN tells us that it *could* be a valid DMA target though. The VM model is capable of using that as a page size, which means we assume it is and want to generate a fault.

Quoted 3 lines

TAGET_PAGE_ALIGN tells us that it *could* be a valid DMA target though. The VM model is capable of using that as a page size, which means we assume it is and want to generate a fault.

We seem to have looped back. So...
It is possible to fix this according to this assumption. In this
case we would need to make TARGET_PAGE_BITS a variable. If we are emulating ancient armv5te, it will be set to 10. For modern targets, ARMv6 and newer, it will be 12.

Quoted 9 lines · Pavel Fedin

Quoted 3 lines

TAGET_PAGE_ALIGN tells us that it *could* be a valid DMA target though. The VM model is capable of using that as a page size, which means we assume it is and want to generate a fault.

We seem to have looped back. So...
It is possible to fix this according to this assumption. In this
case we would need to make TARGET_PAGE_BITS a variable. If we are emulating ancient armv5te, it will be set to 10. For modern targets, ARMv6 and newer, it will be 12.

You can't just make TARGET_PAGE_BITS a variable, it is used as a compile time constant in a bunch of TCG internal stuff. It would be nice if we didn't require it to be compile time, but it would be a lot of work to fix (especially if you want to avoid it being a performance hit).

In any case, that still doesn't fix the problem. On an AArch64 target CPU, TARGET_PAGE_BITS still has to be 12 (for a 4K minimum page size), but the guest and host could still be using 64K pages. So your VFIO code *must* be able to deal with the situation where TARGET_PAGE_BITS is smaller than any alignment that the guest, host or IOMMU need to care about.

I still think the VFIO code needs to figure out what alignment it actually cares about and find some way to determine what that is, or alternatively if the relevant alignment is not possible to determine, write the code so that it doesn't need to care. Either way, TARGET_PAGE_ALIGN is not the answer.

So I think in the end, the one page size we care about is the minimum IOMMU granularity. We don't really care about the target page size at all and maybe we only care about the host page size for determining what might share a page with a sub-page mapping.

Hello!

Quoted 11 lines

Quoted 8 lines

On some architectures TARGET_PAGE_ALIGN() is not enough to get the right alignment. For example on ARM TARGET_PAGE_BITS is 10 because some old CPUs support 1K page size, while minimum SMMU page size is 4K.

This fixes problems like:

2015-11-17T07:37:42.892265Z qemu-system-aarch64: VFIO_MAP_DMA: -22
2015-11-17T07:37:42.892309Z qemu-system-aarch64: vfio_dma_map(0x223da230, 0x80002f0400,

0x10fc00, 0x7f89b40400) = -22 (Invalid

Quoted 2 lines

argument)
qemu: hardware error: vfio: DMA mapping failed, unable to continue

[skip]

Quoted 7 lines

I don't understand how this is supposed to work, if we align to a larger size than the processor, then there are processor size pages of RAM than could be handed out as DMA targets for devices, but we can't map them through the IOMMU. Thus if the guest tries to use them, we get IOMMU faults in the host and likely memory corruption in the guest because the device can't read or write to the page it's supposed to. This doesn't seem like the right solution.

Well, this was my first try on the problem. I've got your idea. But i guess we should discuss the proper solution then.
So, i've got this problem on ARM64. On ARM64 we actually can never have 1K pages. This page size was supported only by old 32-bit ARM CPUs, up to ARMv5 IIRC, then it was dropped. Linux OS never even used it. But, since qemu can emulate those ancient CPUs, TARGET_PAGE_BITS is defined to 10 for ARM. And, ARM64 and ARM32 is actually the same target for qemu, so this is why we still get it. Perhaps, TARGET_PAGE_BITS should be a variable for ARM, and we should
set it according to the actual used CPU. Then this IOMMU alignment
problem would disappear automatically. What do you think?

Cc'ed Peter since he is the main ARM guy here.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Quoted 3 lines · Pavel Fedin

On some architectures TARGET_PAGE_ALIGN() is not enough to get the right alignment. For example on ARM TARGET_PAGE_BITS is 10 because some old CPUs support 1K page size, while minimum SMMU page size is 4K.

I don't understand how this is supposed to work, if we align to a larger size than the processor, then there are processor size pages of RAM than could be handed out as DMA targets for devices, but we can't map them through the IOMMU. Thus if the guest tries to use them, we get IOMMU faults in the host and likely memory corruption in the guest because the device can't read or write to the page it's supposed to. This doesn't seem like the right solution. Thanks,

There are a number of different interesting page sizes here:
* the host kernel page size
* the target CPU architecture's worst-case smallest page size
* the page size the guest kernel is actually using at the moment (consider a 4K-page guest kernel on a 64K-page host kernel)

These don't necessarily have to all be the same. I would expect VFIO to be interested in the host kernel page size, not TARGET_PAGE_ALIGN.

Hello!

Quoted 6 lines

There are a number of different interesting page sizes here:
* the host kernel page size
* the target CPU architecture's worst-case smallest page size
* the page size the guest kernel is actually using at the moment (consider a 4K-page guest kernel on a 64K-page host kernel)

Quoted 3 lines

These don't necessarily have to all be the same. I would expect VFIO to be interested in the host kernel page size, not TARGET_PAGE_ALIGN.

So would the appropriate fix be just replacing TARGET_PAGE_ALIGN with HOST_PAGE_ALIGN?
Alex: there's however, still no answer to "why are we aligning up, not down?" question..