Re: [PATCH v5 04/13] xen: extend domctl interface for cache coloring

[Julien Grall <julien@xxxxxxx>]

Hi Carlo,

On 08/01/2024 16:31, Carlo Nonato wrote:
> On Mon, Jan 8, 2024 at 4:32 PM Julien Grall <julien@xxxxxxx> wrote:
> > Hi,
> >
> > On 08/01/2024 15:18, Carlo Nonato wrote:
> > > > No. I am saying that that we should not be able to allow changing the
> > > > colors after the memory has been allocated. To give an example, your
> > > > current code would allow:
> > > >
> > > >      1) Setup the P2M pools or allocate RAM
> > > >      2) Set the color
> > > >
> > > > This would render the coloring configuration moot.
> > > >
> > > > Whether we want to allow changing the coloring before hand is a
> > > > different question and as I wrote earlier on, I don't mind if you want
> > > > to forbid that.
> > > At the moment I'm relying on the toolstack in the sense that I know that it
> > > will set colors right after domain creation and before memory allocation.
> > > Calling alloc_domheap_pages() without a coloring configuration makes Xen
> > > crash, so it's mandatory to have the configuration done before any allocation.
> > > I know that we shouldn't rely on the toolstack this much, but I didn't
> > > find a better way. Given this assumption, looking for an already existing
> > > color configuration of a domain is sufficient to avoid what you are saying.
> > >
> > > Is it possible to enforce such a constraint with domctl? > I mean to be sure 
> > > that this domctl will be called at a precise time.
> > Yes. You can...
> >
> > > Thanks.
> > >
> > > > > I don't know what to check that.
> > > > You can check the size of the P2M pool (d->arch.paging.p2m_total_pages)
> > > > is still 0. I think for RAM, you can check d->tot_pages == 0.
> > ... reject the call if either of the two fields are not zero.
> What I'm saying is that Xen would crash before even reaching this point if no
> colors were provided. Let's say that the toolstack or whatever hypercall user
> isn't calling this domctl at all (or not at the right time), then the domain
> colored allocator would always return null pages since there are no colors.
> We would have a crash and your if (or mine) would be useless.
Really? I can believe that NULL may be returned but a crash... If that's 
the case, then this should be fixed.
> Let's say that now the domctl is called at the right time (no p2m,
> no tot_pages, no colors) then we can set the colors and everything works.
>  From this point other calls to this domctl would be skipped because of your
> if which is equivalent to mine (checking for colors existence).
So I misunderstood the implementation of is_domain_llc_colored(). I 
would have thought it was based on whether the domain has colors. But 
instead, it is based on whether coloring is enabled globally.
So I agree that the checks are not necessary today. But if 
is_domain_llc_colored() is changed, then this may not be correct anymore.
So I think there are some clarifications required. If the 'd' will never 
matter, then probably is_domain_llc_colored() should be removed. If not, 
then we want to add the check in the domctl (or at minimum document it).
> Also bringing in checks on p2m would require arch specific code which I was
> trying to avoid.
Fair enough. But the first step is to make the sure the code is correct 
and Xen doesn't crash. We can then discuss about avoiding arch specific 
code.
BTW, all your LLC code is implemented in arch/arm. So if it is really 
intended to contain zero arch specific code, then shouldn't it be 
implemented in common/?
Cheers,

--
Julien Grall