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

["Cédric Le Goater" <clegoate@redhat.com>]

On 12/9/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)

This is duplicating s390_pci_iommu_enable(). May be we could add a new
argument to s390_pci_iommu_enable() instead ?

> +{
> +    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)

This is very much like s390_pci_batch_unmap(). Could we introduce a
common helper ?

> +{
> +    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;
> +    }
> +}
> +
>   static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
>                       uintptr_t ra)
>   {
> @@ -1008,7 +1036,7 @@ static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
>       }
>   
>       /* currently we only support designation type 1 with translation */
> -    if (!(dt == ZPCI_IOTA_RTTO && t)) {
> +    if (t && !(dt == ZPCI_IOTA_RTTO)) {

Is this change part of the patchset ? It looks like some other issue.
I might be wrong.

>           error_report("unsupported ioat dt %d t %d", dt, t);
>           s390_program_interrupt(env, PGM_OPERAND, ra);
>           return -EINVAL;
> @@ -1018,13 +1046,23 @@ static int reg_ioat(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib,
>       iommu->pal = pal;
>       iommu->g_iota = g_iota;
>   
> -    s390_pci_iommu_enable(iommu);
> +    if (t) {
> +        s390_pci_iommu_enable(iommu);
> +    } else {
> +        s390_pci_iommu_dm_enable(iommu);
> +        /* If not translating, map everything now */
> +        s390_pci_setup_stage2_map(iommu);
> +    }


I don't understand how we can enter "map everything" case.
Could you explain a bit more the scenario ?

Thanks,

C.

>       return 0;
>   }
>   
>   void pci_dereg_ioat(S390PCIIOMMU *iommu)
>   {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    if (iommu->direct_map) {
> +        s390_pci_batch_unmap(iommu, iommu->pba, ms->ram_size);
> +    }
>       s390_pci_iommu_disable(iommu);
>       iommu->pba = 0;
>       iommu->pal = 0;
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index 2c43ea123f..2aa55e3fd0 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -278,6 +278,7 @@ struct S390PCIIOMMU {
>       MemoryRegion mr;
>       IOMMUMemoryRegion iommu_mr;
>       bool enabled;
> +    bool direct_map;
>       uint64_t g_iota;
>       uint64_t pba;
>       uint64_t pal;
> @@ -389,6 +390,7 @@ int pci_chsc_sei_nt2_have_event(void);
>   void s390_pci_sclp_configure(SCCB *sccb);
>   void s390_pci_sclp_deconfigure(SCCB *sccb);
>   void s390_pci_iommu_enable(S390PCIIOMMU *iommu);
> +void s390_pci_iommu_dm_enable(S390PCIIOMMU *iommu);
>   void s390_pci_iommu_disable(S390PCIIOMMU *iommu);
>   void s390_pci_generate_error_event(uint16_t pec, uint32_t fh, uint32_t fid,
>                                      uint64_t faddr, uint32_t e);