Re: [PATCH V3 01/19] x86: Create per-domain mapping of guest_root_pt

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, May 14, 2024 at 06:15:57PM +0100, Elias El Yandouzi wrote:
> Hi Roger,
> 
> On 13/05/2024 16:27, Roger Pau Monné wrote:
> > > diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> > > index 2a445bb17b..1b025986f7 100644
> > > --- a/xen/arch/x86/pv/domain.c
> > > +++ b/xen/arch/x86/pv/domain.c
> > > @@ -288,6 +288,21 @@ static void pv_destroy_gdt_ldt_l1tab(struct vcpu *v)
> > >                                 1U << GDT_LDT_VCPU_SHIFT);
> > >   }
> > > +static int pv_create_root_pt_l1tab(struct vcpu *v)
> > > +{
> > > +    return create_perdomain_mapping(v->domain,
> > > +                                    PV_ROOT_PT_MAPPING_VCPU_VIRT_START(v),
> > > +                                    1, v->domain->arch.pv.root_pt_l1tab,
> > > +                                    NULL);
> > > +}
> > > +
> > > +static void pv_destroy_root_pt_l1tab(struct vcpu *v)
> > 
> > The two 'v' parameters could be const here.
> 
> I could constify the parameters but the functions wouldn't be consistent
> with the two above for gdt/ldt.

The fact they are not const for the other helpers would also need
fixing at some point IMO, it's best if those are already using the
correct type.

> > > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > > index df015589ce..c1377da7a5 100644
> > > --- a/xen/arch/x86/x86_64/entry.S
> > > +++ b/xen/arch/x86/x86_64/entry.S
> > > @@ -162,7 +162,15 @@ FUNC_LOCAL(restore_all_guest)
> > >           and   %rsi, %rdi
> > >           and   %r9, %rsi
> > >           add   %rcx, %rdi
> > > +
> > > +        /*
> > > +         * The address in the vCPU cr3 is always mapped in the per-domain
> > > +         * pv_root_pt virt area.
> > > +         */
> > > +        imul  $PAGE_SIZE, VCPU_id(%rbx), %esi
> > 
> > Aren't some of the previous operations against %rsi now useless since
> > it gets unconditionally overwritten here?
> 
> I think I can just get rid off of:
> 
>     and   %r9, %rsi
> 
> > and   %r9, %rsi
> > [...]
> > add   %rcx, %rsi
> 
> The second operation you suggested is actually used to retrieve the VA of
> the PV_ROOT_PT.

Oh, yes, sorry, got confused when looking at the source file together
with the diff, it's only the `and` that can be removed.

Thanks, Roger.