lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3db23e11-5fac-f36e-c89e-93c4e8ac2d98@arm.com>
Date:   Fri, 10 Mar 2023 12:04:21 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Peter Geis <pgwipeout@...il.com>,
        Lucas Tanure <lucas.tanure@...labora.com>
Cc:     Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczynski <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>, Qu Wenruo <wqu@...e.com>,
        Piotr Oniszczuk <piotr.oniszczuk@...il.com>,
        Kever Yang <kever.yang@...k-chips.com>,
        linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH 1/7] irqchip/gic-v3: Add a DMA Non-Coherent flag

On 2023-03-10 11:41, Peter Geis wrote:
> On Fri, Mar 10, 2023 at 3:05 AM Lucas Tanure <lucas.tanure@...labora.com> wrote:
>>
>> The GIC600 integration in RK356x, used in rk3588, doesn't support
>> any of the shareability or cacheability attributes, and requires
>> both values to be set to 0b00 for all the ITS and Redistributor
>> tables.
>>
>> This is loosely based on prior work from XiaoDong Huang and
>> Peter Geis fixing this issue specifically for Rockchip 356x.
> 
> Good Morning,
> 
> Since the gic is using dma, would it be reasonable to have all memory
> allocations be requested with the GFP_DMA flag? Otherwise this doesn't
> fully solve the problem for rk356x, where only the lower 4GB range is
> DMA capable, but this tends to get allocated in the upper 4GB on 8GB
> boards.

Not really, because there's no fixed definition of what GFP_DMA actually 
means, and it may mean nothing (same for GFP_DMA32, which may or may not 
be meaningful depending on kernel config and platform topology). Drivers 
should really use the DMA API allocation functions if they care about 
what they get, which comes back round to the notion from years ago of 
converting the ITS driver to a regular platform driver, so it can 
benefit from regular DT concepts like "dma-ranges" automatically.

Thanks,
Robin.

> 
> Very Respectfully,
> Peter Geis
> 
>>
>> Suggested-by: Robin Murphy <robin.murphy@....com>
>> Signed-off-by: Lucas Tanure <lucas.tanure@...labora.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 973ede0197e3..1c334dfeb647 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -42,6 +42,7 @@
>>   #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING          (1ULL << 0)
>>   #define ITS_FLAGS_WORKAROUND_CAVIUM_22375      (1ULL << 1)
>>   #define ITS_FLAGS_WORKAROUND_CAVIUM_23144      (1ULL << 2)
>> +#define ITS_FLAGS_DMA_NON_COHERENT             (1ULL << 3)
>>
>>   #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING    (1 << 0)
>>   #define RDIST_FLAGS_RD_TABLES_PREALLOCATED     (1 << 1)
>> @@ -2359,6 +2360,13 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
>>          its_write_baser(its, baser, val);
>>          tmp = baser->val;
>>
>> +       if (its->flags & ITS_FLAGS_DMA_NON_COHERENT) {
>> +               if (tmp & GITS_BASER_SHAREABILITY_MASK)
>> +                       tmp &= ~GITS_BASER_SHAREABILITY_MASK;
>> +               else
>> +                       gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
>> +       }
>> +
>>          if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) {
>>                  /*
>>                   * Shareability didn't stick. Just use
>> @@ -3055,6 +3063,7 @@ static u64 its_clear_vpend_valid(void __iomem *vlpi_base, u64 clr, u64 set)
>>
>>   static void its_cpu_init_lpis(void)
>>   {
>> +       struct its_node *its = list_first_entry(&its_nodes, struct its_node, entry);
>>          void __iomem *rbase = gic_data_rdist_rd_base();
>>          struct page *pend_page;
>>          phys_addr_t paddr;
>> @@ -3096,6 +3105,9 @@ static void its_cpu_init_lpis(void)
>>          gicr_write_propbaser(val, rbase + GICR_PROPBASER);
>>          tmp = gicr_read_propbaser(rbase + GICR_PROPBASER);
>>
>> +       if (its->flags & ITS_FLAGS_DMA_NON_COHERENT)
>> +               tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK;
>> +
>>          if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) {
>>                  if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) {
>>                          /*
>> @@ -3120,6 +3132,9 @@ static void its_cpu_init_lpis(void)
>>          gicr_write_pendbaser(val, rbase + GICR_PENDBASER);
>>          tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER);
>>
>> +       if (its->flags & ITS_FLAGS_DMA_NON_COHERENT)
>> +               tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK;
>> +
>>          if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) {
>>                  /*
>>                   * The HW reports non-shareable, we must remove the
>> @@ -5005,6 +5020,7 @@ static int __init its_compute_its_list_map(struct resource *res,
>>   static int __init its_probe_one(struct resource *res,
>>                                  struct fwnode_handle *handle, int numa_node)
>>   {
>> +       struct device_node *np = to_of_node(handle);
>>          struct its_node *its;
>>          void __iomem *its_base;
>>          u64 baser, tmp, typer;
>> @@ -5076,6 +5092,9 @@ static int __init its_probe_one(struct resource *res,
>>          its->get_msi_base = its_irq_get_msi_base;
>>          its->msi_domain_flags = IRQ_DOMAIN_FLAG_MSI_REMAP;
>>
>> +       if (np && !of_dma_is_coherent(np))
>> +               its->flags |= ITS_FLAGS_DMA_NON_COHERENT;
>> +
>>          its_enable_quirks(its);
>>
>>          err = its_alloc_tables(its);
>> @@ -5095,6 +5114,9 @@ static int __init its_probe_one(struct resource *res,
>>          gits_write_cbaser(baser, its->base + GITS_CBASER);
>>          tmp = gits_read_cbaser(its->base + GITS_CBASER);
>>
>> +       if (its->flags & ITS_FLAGS_DMA_NON_COHERENT)
>> +               tmp &= ~GITS_CBASER_SHAREABILITY_MASK;
>> +
>>          if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) {
>>                  if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) {
>>                          /*
>> --
>> 2.39.2
>>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ