Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing

[Jon Nettleton <jon@solid-run.com>]

On Mon, Sep 6, 2021 at 7:44 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-05 17:03, Lorenzo Pieralisi wrote:
> > On Thu, Aug 05, 2021 at 09:07:17AM +0100, Shameer Kolothum wrote:
> >
> > [...]
> >
> >> +static void __init iort_node_get_rmr_info(struct acpi_iort_node *iort_node)
> >> +{
> >> +    struct acpi_iort_node *smmu;
> >> +    struct acpi_iort_rmr *rmr;
> >> +    struct acpi_iort_rmr_desc *rmr_desc;
> >> +    u32 map_count = iort_node->mapping_count;
> >> +    u32 sid;
> >> +    int i;
> >> +
> >> +    if (!iort_node->mapping_offset || map_count != 1) {
> >> +            pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> >> +                   iort_node);
> >> +            return;
> >> +    }
> >> +
> >> +    /* Retrieve associated smmu and stream id */
> >> +    smmu = iort_node_get_id(iort_node, &sid, 0);
> >> +    if (!smmu) {
> >> +            pr_err(FW_BUG "Invalid SMMU reference, skipping RMR node %p\n",
> >> +                   iort_node);
> >> +            return;
> >> +    }
> >> +
> >> +    /* Retrieve RMR data */
> >> +    rmr = (struct acpi_iort_rmr *)iort_node->node_data;
> >> +    if (!rmr->rmr_offset || !rmr->rmr_count) {
> >> +            pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR node %p\n",
> >> +                   iort_node);
> >> +            return;
> >> +    }
> >> +
> >> +    rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_node,
> >> +                            rmr->rmr_offset);
> >> +
> >> +    iort_rmr_desc_check_overlap(rmr_desc, rmr->rmr_count);
> >> +
> >> +    for (i = 0; i < rmr->rmr_count; i++, rmr_desc++) {
> >> +            struct iommu_resv_region *region;
> >> +            enum iommu_resv_type type;
> >> +            int prot = IOMMU_READ | IOMMU_WRITE;
> >> +            u64 addr = rmr_desc->base_address, size = rmr_desc->length;
> >> +
> >> +            if (!IS_ALIGNED(addr, SZ_64K) || !IS_ALIGNED(size, SZ_64K)) {
> >> +                    /* PAGE align base addr and size */
> >> +                    addr &= PAGE_MASK;
> >> +                    size = PAGE_ALIGN(size + offset_in_page(rmr_desc->base_address));
> >> +
> >> +                    pr_err(FW_BUG "RMR descriptor[0x%llx - 0x%llx] not aligned to 64K, continue with [0x%llx - 0x%llx]\n",
> >> +                           rmr_desc->base_address,
> >> +                           rmr_desc->base_address + rmr_desc->length - 1,
> >> +                           addr, addr + size - 1);
> >> +            }
> >> +            if (rmr->flags & IOMMU_RMR_REMAP_PERMITTED) {
> >> +                    type = IOMMU_RESV_DIRECT_RELAXABLE;
> >> +                    /*
> >> +                     * Set IOMMU_CACHE as IOMMU_RESV_DIRECT_RELAXABLE is
> >> +                     * normally used for allocated system memory that is
> >> +                     * then used for device specific reserved regions.
> >> +                     */
> >> +                    prot |= IOMMU_CACHE;
> >> +            } else {
> >> +                    type = IOMMU_RESV_DIRECT;
> >> +                    /*
> >> +                     * Set IOMMU_MMIO as IOMMU_RESV_DIRECT is normally used
> >> +                     * for device memory like MSI doorbell.
> >> +                     */
> >> +                    prot |= IOMMU_MMIO;
> >> +            }
> >
> > On the prot value assignment based on the remapping flag, I'd like to
> > hear Robin/Joerg's opinion, I'd avoid being in a situation where
> > "normally" this would work but then we have to quirk it.
> >
> > Is this a valid assumption _always_ ?
>
> No. Certainly applying IOMMU_CACHE without reference to the device's
> _CCA attribute or how CPUs may be accessing a shared buffer could lead
> to a loss of coherency. At worst, applying IOMMU_MMIO to a
> device-private buffer *could* cause the device to lose coherency with
> itself if the memory underlying the RMR may have allocated into system
> caches. Note that the expected use for non-remappable RMRs is the device
> holding some sort of long-lived private data in system RAM - the MSI
> doorbell trick is far more of a niche hack really.
>
> At the very least I think we need to refer to the device's memory access
> properties here.
>
> Jon, Laurentiu - how do RMRs correspond to the EFI memory map on your
> firmware? I'm starting to think that as long as the underlying memory is
> described appropriately there then we should be able to infer correct
> attributes from the EFI memory type and flags.

The devices are all cache coherent and marked as _CCA, 1.  The Memory
regions are in the virt table as ARM_MEMORY_REGION_ATTRIBUTE_DEVICE.

The current chicken and egg problem we have is that during the fsl-mc-bus
initialization we call

error = acpi_dma_configure_id(&pdev->dev, DEV_DMA_COHERENT,
                                              &mc_stream_id);

which gets deferred because the SMMU has not been initialized yet. Then we
initialize the RMR tables but there is no device reference there to be able to
query device properties, only the stream id.  After the IORT tables are parsed
and the SMMU is setup, on the second device probe we associate everything
based on the stream id and the fsl-mc-bus device is able to claim its 1-1 DMA
mappings.

cat /sys/kernel/iommu_groups/0/reserved_regions
0x0000000001000000 0x0000000010ffffff direct-relaxable
0x0000000008000000 0x00000000080fffff msi
0x000000080c000000 0x000000081bffffff direct-relaxable
0x0000001c00000000 0x0000001c001fffff direct-relaxable
0x0000002080000000 0x000000209fffffff direct-relaxable

-Jon

>
> Robin.