Re: [Xen-devel] [PATCH 14/17] xen/arm64: head: Remove ID map as soon as it is not used

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 30 Jul 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 7/30/19 6:33 PM, Stefano Stabellini wrote:
> > On Thu, 27 Jun 2019, Julien Grall wrote:
> > > On 6/27/19 7:55 PM, Stefano Stabellini wrote:
> > > > On Mon, 10 Jun 2019, Julien Grall wrote:
> > > > > +1:
> > > > > +        /*
> > > > > +         * Find the second slot used. Remove the entry for the first
> > > > > +         * table if the slot is not 1 (runtime Xen mapping is 2M -
> > > > > 4M).
> > > > > +         * For slot 1, it means the ID map was not created.
> > > > > +         */
> > > > > +        lsr   x1, x19, #SECOND_SHIFT
> > > > > +        and   x1, x1, #LPAE_ENTRY_MASK  /* x1 := first slot */
> > > > > +        cmp   x1, #1
> > > > > +        beq   id_map_removed
> > > > > +        /* It is not in slot 1, remove the entry */
> > > > > +        ldr   x0, =boot_second          /* x0 := second table */
> > > > > +        str   xzr, [x0, x1, lsl #3]
> > > > 
> > > > Wouldn't it be a bit more reliable if we checked whether the slot in
> > > > question for x19 (whether zero, first, second) is a pagetable pointer or
> > > > section map, then zero it if it is a section map, otherwise go down one
> > > > level? If we did it this way it would be independent from the way
> > > > create_page_tables is written.
> > > 
> > > Your suggestion will not comply with the architecture compliance and how
> > > Xen
> > > is/will be working after the full rework. We want to remove everything
> > > (mapping + table) added specifically for the 1:1 mapping.
> > > 
> > > Otherwise, you may end up in a position where boot_first_id is still in
> > > place.
> > > We would need to use the break-before-make sequence in subsequent code if
> > > we
> > > were about to insert 1GB mapping at the same place.
> > > 
> > > After my rework, we would have virtually no place where break-before-make
> > > will
> > > be necessary as it will enforce all the mappings to be destroyed before
> > > hand.
> > > So I would rather avoid to make a specific case for the 1:1 mapping.
> > 
> > I don't fully understand your explanation. I understand the final goal
> > of "removing everything (mapping + table) added specifically for the 1:1
> > mapping". I don't understand why my suggestion would be a hindrance
> > toward that goal, compared to what it is done in this patch.
> 
> Because, AFAICT, your suggestion will only remove the mapping and not the
> tables (such as boot_first_id). This is different from this patch where both
> mapping and tables are removed.
>
> So yes, my suggestion is not generic, but at least it does the job that is
> expected by this function. I.e removing anything that was specifically created
> for the identity mapping.

I understand your comment now, and of course I agree that both mapping
and tables need to be removed.

I am careful making suggestions for assembly coding because I don't
really want to suggest something that doesn't work, or even if it works
that it's worse than the original.

It should be possible to remove both the table and the mapping in a
generic way. Instead of hardcoding the assemply equivalent of "It is not
in slot 0, remove the entry", we could check whether the table offset
matches the table offset of the mapping that we want to preserve. That
way, "slot 0" would be calculate instead of hardcoded, and the code
would be pretty generic. What do you think? It should only be a small
addition.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel