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

[Elias El Yandouzi <eliasely@xxxxxxxxxx>]

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.
> > @@ -381,6 +412,11 @@ int pv_domain_initialise(struct domain *d)
> >       if ( rc )
> >           goto fail;
> >   
> > +    rc = create_perdomain_mapping(d, PV_ROOT_PT_MAPPING_VIRT_START,
> > +                                  PV_ROOT_PT_MAPPING_ENTRIES, NULL, NULL);
> Why not use d->max_vcpus here, instead of forcing up to MAX_VIRT_CPUS?
>
> By the time pv_domain_initialise() is called max_vcpus should already
> be initialized.  AFAICT it doesn't make a difference, because for the
> call here only the L3 table is created, as last two parameters are
> NULL, but still is more accurate to use max_vcpus.

There is no particular reason. I think we can safely use d->max_vcpus.
> > 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.
Cheers,
Elias