[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/4] xen/arm: its: collect quirk flags and honor dma-noncoherent
On 3/25/26 17:47, Mykola Kvach wrote:
Hi Volodymyr,
Hello Mykola and Volodymyr
Thank you for the review.
On Wed, Mar 25, 2026 at 4:42 PM Volodymyr Babchuk
<Volodymyr_Babchuk@xxxxxxxx> wrote:
Hi Mykola,
Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
From: Mykola Kvach <mykola_kvach@xxxxxxxx>
Replace the per-quirk init callback with declarative flags in
struct its_quirk, and introduce gicv3_its_collect_quirks() to gather
the effective workaround flags from both the IIDR-matched quirk entry
and the "dma-noncoherent" device-tree property.
This lets non-coherent platforms force non-cacheable ITS table
attributes even when no IIDR quirk entry matches.
Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
---
xen/arch/arm/gic-v3-its.c | 70 ++++++++++++++++++++++++---------------
1 file changed, 43 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 9ba068c46f..00524b43a3 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -57,71 +57,87 @@ struct its_device {
*/
struct its_quirk {
const char *desc;
- bool (*init)(struct host_its *hw_its);
uint32_t iidr;
uint32_t mask;
+ uint32_t flags;
};
static uint32_t __ro_after_init its_quirk_flags;
-static bool gicv3_its_enable_quirk_gen4(struct host_its *hw_its)
-{
- its_quirk_flags |= HOST_ITS_WORKAROUND_NC_NS |
- HOST_ITS_WORKAROUND_32BIT_ADDR;
-
- return true;
-}
-
static const struct its_quirk its_quirks[] = {
{
- .desc = "R-Car Gen4",
- .iidr = 0x0201743b,
- .mask = 0xffffffffU,
- .init = gicv3_its_enable_quirk_gen4,
+ .desc = "R-Car Gen4",
+ .iidr = 0x0201743b,
+ .mask = 0xffffffffU,
+ .flags = HOST_ITS_WORKAROUND_NC_NS |
+ HOST_ITS_WORKAROUND_32BIT_ADDR,
},
{
/* Sentinel. */
}
};
-static struct its_quirk* gicv3_its_find_quirk(uint32_t iidr)
+static const struct its_quirk *gicv3_its_find_quirk(uint32_t iidr)
{
- const struct its_quirk *quirks = its_quirks;
+ const struct its_quirk *quirk = its_quirks;
- for ( ; quirks->desc; quirks++ )
+ for ( ; quirk->desc; quirk++ )
{
- if ( quirks->iidr == (quirks->mask & iidr) )
- return (struct its_quirk *)quirks;
+ if ( quirk->iidr != (quirk->mask & iidr) )
+ continue;
+
+ return quirk;
}
return NULL;
}
-static void gicv3_its_enable_quirks(struct host_its *hw_its)
+static uint32_t gicv3_its_collect_quirks(const struct host_its *hw_its,
+ const struct its_quirk
**matched_quirk)
{
+ const struct its_quirk *quirk;
+ uint32_t flags = 0;
uint32_t iidr = readl_relaxed(hw_its->its_base + GITS_IIDR);
- const struct its_quirk *quirk = gicv3_its_find_quirk(iidr);
- if ( quirk && quirk->init(hw_its) )
+ quirk = gicv3_its_find_quirk(iidr);
+ if ( quirk )
+ flags |= quirk->flags;
+
+ if ( hw_its->dt_node &&
+ dt_property_read_bool(hw_its->dt_node, "dma-noncoherent") )
+ flags |= HOST_ITS_WORKAROUND_NC_NS;
+
+ if ( matched_quirk )
+ *matched_quirk = quirk;
+
+ return flags;
+}
+
+static void gicv3_its_enable_quirks(struct host_its *hw_its)
+{
+ const struct its_quirk *quirk;
+
+ its_quirk_flags = gicv3_its_collect_quirks(hw_its, &quirk);
+
+ if ( quirk )
printk("GICv3: enabling workaround for ITS: %s\n", quirk->desc);
}
static void gicv3_its_validate_quirks(void)
{
- const struct its_quirk *quirk = NULL, *prev = NULL;
+ uint32_t quirks, prev_quirks;
const struct host_its *hw_its;
if ( list_empty(&host_its_list) )
return;
hw_its = list_first_entry(&host_its_list, struct host_its, entry);
- prev = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base + GITS_IIDR));
+ prev_quirks = gicv3_its_collect_quirks(hw_its, NULL);
- list_for_each_entry(hw_its, &host_its_list, entry)
+ list_for_each_entry_continue(hw_its, &host_its_list, entry)
{
- quirk = gicv3_its_find_quirk(readl_relaxed(hw_its->its_base +
GITS_IIDR));
- BUG_ON(quirk != prev);
- prev = quirk;
+ quirks = gicv3_its_collect_quirks(hw_its, NULL);
+ BUG_ON(quirks != prev_quirks);
I know it was in the previous version, but as you are already touching
this... This is not Xen BUG(). This is a platform problem. So you need
to panic here. Something like
if (quirks != prev_quirks)
panic("Different ITS instances has different quirks")
Ack.
I agree that a quirk mismatch is a platform problem. Yes, the current
design uses global flags, making it unable to handle mixed quirks,
leading to the failure on mismatch.
Please note, I am not saying a panic() is wrong here and I am not
requesting any changes here; I was just wondering why this is handled
differently than the SMMUv3 driver. I am just thinking out loud.
SMMUv3 driver handles feature mismatches by gracefully degrading. When
it finds an SMMU device that does not support ARM_SMMU_FEAT_COHERENCY,
it disables that feature for the entire platform (so the P2M code has to
clean the cache when updating ptes). It does not panic. How the ITS and
SMMUv3 drivers are different in that regard? Why could not we apply the
same "worst-case" logic here?
For example:
- if any ITS device requires non-cacheable memory, then all ITS memory
allocations should use non-cacheable memory.
- if any ITS device requires 32-bit addresses, then all ITS memory
allocations should be constrained to 32-bits.
This would be consistent with the SMMU precedent and would allow the
system to boot and function correctly, but with the performance
characteristics of the worst ITS device in the system.
Or I really missed something?
[snip]
|
