On 9/17/2021 2:26 PM, Shameerali Kolothum Thodi wrote: > > >> -----Original Message----- >> From: Jon Nettleton [mailto:jon@xxxxxxxxxxxxx] >> Sent: 16 September 2021 12:17 >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx> >> Cc: Robin Murphy <robin.murphy@xxxxxxx>; Lorenzo Pieralisi >> <lorenzo.pieralisi@xxxxxxx>; Laurentiu Tudor <laurentiu.tudor@xxxxxxx>; >> linux-arm-kernel <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; ACPI Devel Maling >> List <linux-acpi@xxxxxxxxxxxxxxx>; Linux IOMMU >> <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; Will >> Deacon <will@xxxxxxxxxx>; wanghuiqiang <wanghuiqiang@xxxxxxxxxx>; >> Guohanjun (Hanjun Guo) <guohanjun@xxxxxxxxxx>; Steven Price >> <steven.price@xxxxxxx>; Sami Mujawar <Sami.Mujawar@xxxxxxx>; Eric >> Auger <eric.auger@xxxxxxxxxx>; yangyicong <yangyicong@xxxxxxxxxx> >> Subject: Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing >> >> On Thu, Sep 16, 2021 at 10:26 AM Shameerali Kolothum Thodi >> <shameerali.kolothum.thodi@xxxxxxxxxx> wrote: >>> >>> >>> >>>> -----Original Message----- >>>> From: Jon Nettleton [mailto:jon@xxxxxxxxxxxxx] >>>> Sent: 16 September 2021 08:52 >>>> To: Shameerali Kolothum Thodi >> <shameerali.kolothum.thodi@xxxxxxxxxx> >>>> Cc: Robin Murphy <robin.murphy@xxxxxxx>; Lorenzo Pieralisi >>>> <lorenzo.pieralisi@xxxxxxx>; Laurentiu Tudor >>>> <laurentiu.tudor@xxxxxxx>; linux-arm-kernel >>>> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; ACPI Devel Maling List >>>> <linux-acpi@xxxxxxxxxxxxxxx>; Linux IOMMU >>>> <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; >>>> Will Deacon <will@xxxxxxxxxx>; wanghuiqiang >>>> <wanghuiqiang@xxxxxxxxxx>; Guohanjun (Hanjun Guo) >>>> <guohanjun@xxxxxxxxxx>; Steven Price <steven.price@xxxxxxx>; Sami >>>> Mujawar <Sami.Mujawar@xxxxxxx>; Eric Auger >> <eric.auger@xxxxxxxxxx>; >>>> yangyicong <yangyicong@xxxxxxxxxx> >>>> Subject: Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node >>>> parsing >>>> >>>> On Thu, Sep 16, 2021 at 9:26 AM Shameerali Kolothum Thodi >>>> <shameerali.kolothum.thodi@xxxxxxxxxx> wrote: >>>>> >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Jon Nettleton [mailto:jon@xxxxxxxxxxxxx] >>>>>> Sent: 06 September 2021 20:51 >>>>>> To: Robin Murphy <robin.murphy@xxxxxxx> >>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>; Shameerali >>>>>> Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>; Laurentiu >>>>>> Tudor <laurentiu.tudor@xxxxxxx>; linux-arm-kernel >>>>>> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; ACPI Devel Maling List >>>>>> <linux-acpi@xxxxxxxxxxxxxxx>; Linux IOMMU >>>>>> <iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx>; Linuxarm >>>>>> <linuxarm@xxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; Will >>>>>> Deacon <will@xxxxxxxxxx>; wanghuiqiang >>>>>> <wanghuiqiang@xxxxxxxxxx>; Guohanjun (Hanjun Guo) >>>>>> <guohanjun@xxxxxxxxxx>; Steven Price <steven.price@xxxxxxx>; >>>>>> Sami Mujawar <Sami.Mujawar@xxxxxxx>; Eric Auger >>>> <eric.auger@xxxxxxxxxx>; >>>>>> yangyicong <yangyicong@xxxxxxxxxx> >>>>>> Subject: Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node >>>>>> parsing >>>>>> >>>>> [...] >>>>> >>>>>>>> >>>>>>>> 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. >>>>> >>>>> Can we solve this order problem by delaying the >>>>> iommu_alloc_resv_region() to the >>>>> iommu_dma_get_rmr_resv_regions(dev, >>>>> list) ? We could invoke >>>>> device_get_dma_attr() from there which I believe will return the >>>>> _CCA >>>> attribute. >>>>> >>>>> Or is that still early to invoke that? >>>> >>>> That looks like it should work. Do we then also need to parse >>>> through the VirtualMemoryTable matching the start and end addresses >>>> to determine the other memory attributes like MMIO? >>> >>> Yes. But that looks tricky as I can't find that readily available on >>> Arm, like the efi_mem_attributes(). I will take a look. >>> >>> Please let me know if there is one or any other easy way to retrieve it. >> >> maybe we don't need to. Maybe it is enough to just move >> iommu_alloc_resv_regions and then set the IOMMU_CACHE flag if type = >> IOMMU_RESV_DIRECT_RELAXABLE and _CCN=1? > > It looks like we could simply call efi_mem_type() and check for > EFI_MEMORY_MAPPED_IO. I have updated the code to set the > RMR prot value based on _CCA and EFI md type. Please see the > last commit on this branch here(not tested), > > https://github.com/hisilicon/kernel-dev/commits/private-v5.14-rc4-rmr-v7-ext > > Please take a look and let me know if this is good enough to solve this problem. > Sorry for the delay, I managed to test on a NXP LX2160A and things look fine, so: Tested-by: Laurentiu Tudor <laurentiu.tudor@xxxxxxx> --- Best Regards, Laurentiu
