Re: [PATCH v5 13/13] xen/arm: add cache coloring support for Xen

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Julien,

On Mon, Jan 15, 2024 at 5:16 PM Julien Grall <julien@xxxxxxx> wrote:
> On 15/01/2024 15:43, Carlo Nonato wrote:
> > Hi Julien,
>
> Hi Carlo,
>
> > On Mon, Jan 15, 2024 at 12:18 PM Julien Grall <julien@xxxxxxx> wrote:
> >> On 15/01/2024 10:11, Carlo Nonato wrote:
> >>> I understand what you're talking about, and it seems reasonable to get 
> >>> rid of
> >>> xen_colored_temp[] and create_llc_coloring_mappings() since in the end 
> >>> they
> >>> serve the purpose of mapping the physically colored space that is already
> >>> mapped using xen_xenmap[] pagetables.
> >>> What I don't understand is then how to copy/relocate Xen since I don't 
> >>> have a
> >>> destination virtual space anymore to use in relocate_xen().
> >>
> >> You will need to link xen_xenmap[] in boot_second[...] as well. With
> >> that, you will be able to access the new Xen through the temporary area.
> >
> > Wouldn't it result in overwriting the current virtual space mapping?
> > boot_second is the live page table and if I link xen_xenmap[] then
> > XEN_VIRT_START would point to the new colored space which is still empty at
> > this stage...
>
> If you link at XEN_VIRT_START then yes. But you could link at
> BOOT_RELOC_VIRT_START like you already do today.

Ok I think I got it.

> >
> >> [...]
> >>
> >>>> Note that this means the init_ttbr cannot be written directly. But you
> >>>> can solve this problem by re-mapping the address.
> >>>
> >>> How to remap a single address?
> >>
> >> You should be able to use map_domain_page() to map the page where
> >> init_ttbr is.
> >>
> >>> And if moving init_ttbr in the identity-mapped area means that it's no 
> >>> longer
> >>> writable, so that I need to remap it, why moving it in that area in the 
> >>> first
> >>> place. Again I think I'm missing something.
> >>
> >> The goal is to have everything used (code, data) before the MMU is
> >> turned on residing in a single page. So secondary CPUs can directly jump
> >> to the colored Xen without any trouble.
> >
> > This is what confuses me. Why having everything on a single page makes
> > secondary cpus able to jump directly to colored Xen? (also see below)
>
> Because the code running with the MMU off can access easily access
> everything.

This was what I got wrong. Now it's clear.

> >
> >>>>>
> >>>>> 3) To access the identity mapping area I would need some accessor that 
> >>>>> takes
> >>>>> an address and returns it + phys_offset, or is there a better way to do 
> >>>>> it?
> >>>>
> >>>> I am not sure I understand what you mean. Can you clarify?
> >>>
> >>> In my idea, I would use the identity mapping to access the "old" 
> >>> variables,
> >>> where "old" means non physically colored. init_ttbr is an example. When
> >>> Xen it's copied on the new physical space, init_ttbr is copied with it and
> >>> if the boot cpu modifies this variable, it's actually touching the colored
> >>> one and not the old one. This means that secondary CPUs that still haven't
> >>> jumped to the new space, won't be able to see the new value and will never
> >>> go online.
> >>> So to access this "old" init_ttbr variable I need it's identity address,
> >>> which is its current virtual address + some physical offset. I was asking
> >>> you if this is the right approach to use the identity mapping.
> >>
> >> Secondary CPUs would directly start on the colored Xen. So they will be
> >> able to access the "new" init_ttbr & co.
> >
> > How can this be true? I mean, in call_psci_cpu_on() I can start those CPUs 
> > in
> > the colored space, but they still use the boot_* pagetables
>
> Are you looking at the 64-bit or 32-bit code? For 64-bit, staging is not
> using boot_* pagetable anymore for secondary CPUs. Instead, they
> directly jump to the runtime page-tables.

Again, my fault. Got it.

> > and there I can't
> > easily link the new colored space, or, at least, I'm not succeding in doing
> > that. What I tried at the moment is to link xen_xenmap in boot_second after
> > switch_ttbr because of the problem I described above. But then secondary
> > CPUs never go online...
>
> It would be helpful if you share some code.

Given the newfound knowledge, I'll think I can get further.

Thanks.

> >
> >> [...]
> >>
> >>>> ... as I wrote ealier your current approach seems to have a flaw. As you
> >>>> overwrite xen_bootmodule->{start, size}. setup_mm() will end up to add
> >>>> the old Xen region to the boot allocator. This is before any secondary
> >>>> CPUs are booted up.
> >>>>
> >>>> IOW, the allocator may provide some memory from the old Xen and nothing
> >>>> good will happen from that.
> >>>>
> >>>> The only way to solve it is to add another module. So the memory is
> >>>> skipped by setup_mm(). However see below.
> >>>>
> >>>>>
> >>>>> Yes that should be memory that in the end would not be needed so it must
> >>>>> return to the boot-allocator (if that's what you mean). But how to do
> >>>>> that?
> >>>>
> >>>> You can't really discard the old temporary Xen. This may work today
> >>>> because we don't support CPU hotplug or suspend/resume. But there was
> >>>> some series on the ML to enable it and I don't see any reason why
> >>>> someone would not want to use the features with cache coloring.
> >>>>
> >>>> So the old temporary Xen would have to be kept around forever. This is
> >>>> up to 8MB of memory wasted.
> >>>>
> >>>> The right approach is to have the secondary CPU boot code (including the
> >>>> variables it is using) fitting in the same page (or possibly multiple so
> >>>> long this is small and physically contiguous). With that it doesn't
> >>>> matter where is the trampoline, it could stay at the old place, but we
> >>>> would only waste a few pages rather than up 8MB as it is today.
> >>>
> >>> So what are you suggesting is to create a new section in the linker script
> >>> for the trampoline code and data,
> >>
> >> We already have a section for that in place (see .idmap.*) which happens
> >> to be at the beginning of Xen. Right now, the section is in text. Which
> >> is why it is read-only executable.
> >>
> >>> then in setup_mm() we would skip this
> >>> memory?
> >>
> >> We should not need this. Secondary boot CPUs should boot direclty on the
> >> colored Xen.
> >>
> >>> Am I following you correctly? Sorry those topics are a little out
> >>> of my preparation as you probably already guessed.
> >>
> >> No worries. I am happy to go in as much details as necessary. I can also
> >> attempt to write a patch if that helps. (unless someone else in the Arm
> >> maintainers want to give a try).
> >
> > Yes this would help. Thanks.
>
> I will try to have a look this evening. If I can't, it may have to wait
> a couple of weeks unless someone has time before hand.
>
> Cheers,
>
> --
> Julien Grall