On Fri, 16 Feb 2024 08:47:04 +0000,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
>
> Hi Marc,
>
> Thank you for your review!
>
> > > @@ -2201,7 +2202,7 @@ static struct page *its_allocate_prop_table(gfp_t gfp_flags)
> > > {
> > > struct page *prop_page;
> > >
> > > - prop_page = alloc_pages(gfp_flags, get_order(LPI_PROPBASE_SZ));
> > > + prop_page = alloc_pages(gfp_flags | its_gfp_quirk, get_order(LPI_PROPBASE_SZ));
> >
> > This only affects the redistributors. Why should we constraint it?
>
> To be honest, I don't know why, but this is required on my environment.
> Otherwise, the MSI interrupt never happens.
> Anyway, I should have learned the hardware bug in detail...
So the redistributors are also affected by this bug, which makes sense
given the monolithic nature of GIC600.
This should be handled as a separate allocation constraints (i.e. not
using your ITS-specific GFP quirk).
[...]
> > > @@ -2971,7 +2972,7 @@ static struct page *its_allocate_pending_table(gfp_t gfp_flags)
> > > {
> > > struct page *pend_page;
> > >
> > > - pend_page = alloc_pages(gfp_flags | __GFP_ZERO,
> > > + pend_page = alloc_pages(gfp_flags | __GFP_ZERO | its_gfp_quirk,
> > > get_order(LPI_PENDBASE_SZ));
> >
> > This is a redistributor-private allocation. If it is the ITSs that are
> > affected by this bug, why are the pending tables constrained?
>
> Thank you for the comment. This is not needed on my environment.
> So, I'll drop it.
If GICR_PROPBASER needs to be in the low 4GB, it is likely that
GICR_PENDBASER has the same constraint. It is just that the GIC600
caches are big enough that evictions are rare, and that you don't see
a problem. But it is still very likely to exist.
[...]
> > I suggest that you start reading the architecture spec and understand
> > what is the memory that is accessible by the GIC, because at least
> > half of this patch is completely unnecessary.
> >
> > You also need to be clear about what is affected by this bug: ITS
> > only? or also the RDs? If both are affected, they need to be handled
> > separately.
> >
> > In any case, you must not assume that all ITSs in a system are
> > subjected to this bug, which means that you must have per-ITS tracking
> > of the GFP flags.
> >
> > Finally, the DMA story itself needs to be sorted out, because you are
> > relying on something that is incredibly fragile.
>
> Thank you very much for your suggestion. I'll learn the architecture spec
> and reconsider the implementation, especially DMA related.
Using the DMA allocator is an interesting prospect. The only issue
with that is that the ITS isn't currently represented as a device,
which the DMA allocator requires IIRC. You will either have to plumb
something in a low-level allocator, or convert the ITS code to
implement a platform device. The latter won't be fun.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
