Re: [PATCH v3 07/18] x86/hvm: introduce per-vCPU L3 page-table

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Wed, Oct 7, 2026 at 8:21 PM Roger Pau Monné <roger@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Oct 07, 2026 at 11:40:40AM +0100, George Dunlap wrote:
> > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >
> > The per-domain area is currently a single domain-wide structure: one L3,
>
> nit: s/structure/entry/ might be better?

Not sure why?  After this change, there's still a single perdomain
entry; but each vCPU has its own tree, whereas previously there was
only a single tree.  A tree is a structure.

I could change it to "tree" if you want, but I'm not sure that's any clearer.

> > Give init_xen_l4_slots() the vCPU a table is private to, and use
>
> Can't you replace the domain parameter with a vcpu one, and obtain
> domain as v->domain?
[snip]
-
> > Changes in v3:
> > - HVM only: PV domains keep the domain-wide area, so the PV parts go
> >   (the single-vCPU limit, the shadow refusals, the XPTI slot install).
> >   init_xen_l4_slots() takes the vCPU as an extra, optional argument
> >   rather than in place of the domain, so PV guest L4s, which no vCPU
> >   owns, are promoted exactly as before.
>
> Hm, I see, so this is the reason. PV L4s are promoted ahead of being
> loaded into a vCPU %cr3, and hence when init_xen_l4_slots() is called
> the per-vCPU L3 to use is not clear.

Right, so your v2 passed d->vcpu[0] in a couple of places where there
was no current cpu (e.g., dom0 creation).

I think the reason this version ended up this way is that in theory,
the toolstack could do something that would cause promote_l4_table()
to be called before the vcpus are created (by XEN_DOMCTL_max_vcpus).
If we have init_xen_l4_slots() take only a vcpu argument, we'd have to
modify that to check that d->vcpu[0] existed, and fail the call if
not; and any toolstack that did promote an l4 before creating the
vCPUs would stop working.

As it happens, libxc / libxl always call max_vcpus first.  So I think
failing the call would be OK.  Then most of the sites would just take
a single argument, and Jan's question about 32-bit PV guests just goes
away.

I'll give that a try and see how it looks, unless someone has an objection.

> > @@ -525,12 +525,12 @@ int arch_vcpu_create(struct vcpu *v)
> >           */
> >          rc = create_perdomain_mapping(v, PERDOMAIN_VIRT_START, 0, NULL, 
> > NULL);
> >          if ( rc )
> > -            return rc;
> > +            goto fail_early;
> >
> >          paging_vcpu_init(v);
> >
> >          if ( (rc = vcpu_init_fpu(v)) != 0 )
> > -            return rc;
> > +            goto fail_early;
> >
> >          vmce_init_vcpu(v);
> >
> > @@ -578,6 +578,8 @@ int arch_vcpu_create(struct vcpu *v)
> >      vcpu_destroy_fpu(v);
> >      xfree(v->arch.msrs);
> >      v->arch.msrs = NULL;
> > + fail_early:
> > +    free_pervcpu_mappings(v);
>
> Do we really need the extra label?  It would seem to me like the
> existing cleanup functions are harmless to execute regardless of
> whether the matching initialization routine has been called.

Huh, just noticed that all the fail paths call all the cleanup
functions, regardless of whether they need to be cleaned up.  OK,
yeah, we can do the same thing.

> > +    free_perdomain_l3(v->arch.pervcpu_l3_pg);
> > +    v->arch.pervcpu_l3_pg = NULL;
>
> We have been (recently) following the model of first setting the
> pointer to NULL, and then freeing it, ie:
>
> struct page_info *l3_pg = v->arch.pervcpu_l3_pg;
>
> if ( !l3_pg )
>     return;
>
> v->arch.pervcpu_l3_pg = NULL;
> free_perdomain_l3(l3_pg);
>
> Like XFREE() and similar macros.  It's possibly worth using the same
> pattern here.

Ack.

 -George