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

[Laurentiu Tudor <laurentiu.tudor@nxp.com>]

On 9/17/2021 2:26 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Jon Nettleton [mailto:jon@solid-run.com]
>> Sent: 16 September 2021 12:17
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>; Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com>; Laurentiu Tudor <laurentiu.tudor@nxp.com>;
>> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
>> List <linux-acpi@vger.kernel.org>; Linux IOMMU
>> <iommu@lists.linux-foundation.org>; Joerg Roedel <joro@8bytes.org>; Will
>> Deacon <will@kernel.org>; wanghuiqiang <wanghuiqiang@huawei.com>;
>> Guohanjun (Hanjun Guo) <guohanjun@huawei.com>; Steven Price
>> <steven.price@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>; Eric
>> Auger <eric.auger@redhat.com>; yangyicong <yangyicong@huawei.com>
>> 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@huawei.com> wrote:
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jon Nettleton [mailto:jon@solid-run.com]
>>>> Sent: 16 September 2021 08:52
>>>> To: Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>
>>>> Cc: Robin Murphy <robin.murphy@arm.com>; Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com>; Laurentiu Tudor
>>>> <laurentiu.tudor@nxp.com>; linux-arm-kernel
>>>> <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling List
>>>> <linux-acpi@vger.kernel.org>; Linux IOMMU
>>>> <iommu@lists.linux-foundation.org>; Joerg Roedel <joro@8bytes.org>;
>>>> Will Deacon <will@kernel.org>; wanghuiqiang
>>>> <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo)
>>>> <guohanjun@huawei.com>; Steven Price <steven.price@arm.com>; Sami
>>>> Mujawar <Sami.Mujawar@arm.com>; Eric Auger
>> <eric.auger@redhat.com>;
>>>> yangyicong <yangyicong@huawei.com>
>>>> 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@huawei.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jon Nettleton [mailto:jon@solid-run.com]
>>>>>> Sent: 06 September 2021 20:51
>>>>>> To: Robin Murphy <robin.murphy@arm.com>
>>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Shameerali
>>>>>> Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; Laurentiu
>>>>>> Tudor <laurentiu.tudor@nxp.com>; linux-arm-kernel
>>>>>> <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling List
>>>>>> <linux-acpi@vger.kernel.org>; Linux IOMMU
>>>>>> <iommu@lists.linux-foundation.org>; Linuxarm
>>>>>> <linuxarm@huawei.com>; Joerg Roedel <joro@8bytes.org>; Will
>>>>>> Deacon <will@kernel.org>; wanghuiqiang
>>>>>> <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo)
>>>>>> <guohanjun@huawei.com>; Steven Price <steven.price@arm.com>;
>>>>>> Sami Mujawar <Sami.Mujawar@arm.com>; Eric Auger
>>>> <eric.auger@redhat.com>;
>>>>>> yangyicong <yangyicong@huawei.com>
>>>>>> 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@nxp.com>

---
Best Regards, Laurentiu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel