Re: [PATCH 1/2] s390x/pci: add support for guests that request direct mapping

[David Hildenbrand <david@redhat.com>]

On 09.12.24 20:29, Matthew Rosato wrote:
> When receiving a guest mpcifc(4) or mpcifc(6) instruction without the T
> bit set, treat this as a request to perform direct mapping instead of
> address translation.  In order to facilitiate this, pin the entirety of
> guest memory into the host iommu.
> 
> Subsequent guest DMA operations are all expected to be of the format
> guest_phys+sdma, allowing them to be used as lookup into the host
> iommu table.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-bus.c         | 23 ++++++++++++++++++
>   hw/s390x/s390-pci-inst.c        | 42 +++++++++++++++++++++++++++++++--
>   include/hw/s390x/s390-pci-bus.h |  2 ++
>   3 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 40b2567aa7..8d4224e032 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -18,6 +18,7 @@
>   #include "hw/s390x/s390-pci-inst.h"
>   #include "hw/s390x/s390-pci-kvm.h"
>   #include "hw/s390x/s390-pci-vfio.h"
> +#include "hw/boards.h"
>   #include "hw/pci/pci_bus.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/pci/pci_bridge.h"
> @@ -720,6 +721,27 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
>                                TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
>                                name, iommu->pal + 1);
>       iommu->enabled = true;
> +    iommu->direct_map = false;
> +    memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr));
> +    g_free(name);
> +}
> +
> +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +
> +    /*
> +     * For direct-mapping we must map the entire guest address space.  Because
> +     * the mappings are contiguous we are not restricted to individual 4K
> +     * mappings via vfio, so let's not worry about the DMA limit when
> +     * calculating the range.
> +     */
> +    char *name = g_strdup_printf("iommu-s390-%04x", iommu->pbdev->uid);
> +    memory_region_init_iommu(&iommu->iommu_mr, sizeof(iommu->iommu_mr),
> +                             TYPE_S390_IOMMU_MEMORY_REGION, OBJECT(&iommu->mr),
> +                             name, iommu->pba + ms->ram_size);
> +    iommu->enabled = true;
> +    iommu->direct_map = true;
>       memory_region_add_subregion(&iommu->mr, 0, MEMORY_REGION(&iommu->iommu_mr));
>       g_free(name);
>   }
> @@ -727,6 +749,7 @@ void s390_pci_iommu_enable(S390PCIIOMMU *iommu)
>   void s390_pci_iommu_disable(S390PCIIOMMU *iommu)
>   {
>       iommu->enabled = false;
> +    iommu->direct_map = false;
>       g_hash_table_remove_all(iommu->iotlb);
>       memory_region_del_subregion(&iommu->mr, MEMORY_REGION(&iommu->iommu_mr));
>       object_unparent(OBJECT(&iommu->iommu_mr));
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 41655082da..f4d8fe8fe8 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -16,6 +16,7 @@
>   #include "exec/memory.h"
>   #include "qemu/error-report.h"
>   #include "sysemu/hw_accel.h"
> +#include "hw/boards.h"
>   #include "hw/pci/pci_device.h"
>   #include "hw/s390x/s390-pci-inst.h"
>   #include "hw/s390x/s390-pci-bus.h"
> @@ -990,6 +991,33 @@ int pci_dereg_irqs(S390PCIBusDevice *pbdev)
>       return 0;
>   }
>   
> +static void s390_pci_setup_stage2_map(S390PCIIOMMU *iommu)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    uint64_t remain = ms->ram_size, start = iommu->pba, mask, size, curr = 0;
> +    uint64_t end = start + remain - 1;
> +    IOMMUTLBEvent event = {
> +        .type = IOMMU_NOTIFIER_MAP,
> +        .entry = {
> +            .target_as = &address_space_memory,
> +            .translated_addr = 0,
> +            .perm = IOMMU_RW,
> +        },
> +    };
> +
> +    while (remain >= TARGET_PAGE_SIZE) {
> +        mask = dma_aligned_pow2_mask(start, end, 64);
> +        size = mask + 1;
> +        event.entry.iova = start;
> +        event.entry.addr_mask = mask;
> +        event.entry.translated_addr = curr;
> +        memory_region_notify_iommu(&iommu->iommu_mr, 0, event);
> +        start += size;
> +        curr += size;
> +        remain -= size;
> +    }
> +}

Hi,

Trying to wrap my head around that ... you mention that "pin the 
entirety of guest memory".

Do you mean that we will actually end up longterm pinning all guest RAM 
in the kernel, similar to what vfio ends up doing?

In that case, it would be incompatible with virtio-balloon (and without 
modifications with upcoming virtio-mem). Is there already a mechanism in 
place to handle that -- a call  to ram_block_discard_disable() -- or 
even a way to support coordinated discarding of RAM (e.g., virtio-mem + 
vfio)?

-- 
Cheers,

David / dhildenb