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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----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.

Thanks,
Shameer