On Tue, Oct 03, 2023 at 09:53:11AM -0400, Tamas K Lengyel wrote: > On Mon, Oct 2, 2023 at 11:13 AM Roger Pau Monne <[email protected]> wrote: > > > > From: Jan Beulich <[email protected]> > > > > In preparation of the introduction of new vCPU operations allowing to > > register the respective areas (one of the two is x86-specific) by > > guest-physical address, add the necessary fork handling (with the > > backing function yet to be filled in). > > > > Signed-off-by: Jan Beulich <[email protected]> > > Signed-off-by: Roger Pau Monné <[email protected]> > > --- > > Changes since v4: > > - Rely on map_guest_area() to populate the child p2m if necessary. > > --- > > xen/arch/x86/mm/mem_sharing.c | 31 +++++++++++++++++++++++++++++++ > > xen/common/domain.c | 7 +++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c > > index 5f8f1fb4d871..99cf001fd70f 100644 > > --- a/xen/arch/x86/mm/mem_sharing.c > > +++ b/xen/arch/x86/mm/mem_sharing.c > > @@ -1641,6 +1641,24 @@ static void copy_vcpu_nonreg_state(struct vcpu > > *d_vcpu, struct vcpu *cd_vcpu) > > hvm_set_nonreg_state(cd_vcpu, &nrs); > > } > > > > +static int copy_guest_area(struct guest_area *cd_area, > > + const struct guest_area *d_area, > > + struct vcpu *cd_vcpu, > > + const struct domain *d) > > +{ > > + unsigned int offset; > > + > > + /* Check if no area to map, or already mapped. */ > > + if ( !d_area->pg || cd_area->pg ) > > + return 0; > > + > > + offset = PAGE_OFFSET(d_area->map); > > + return map_guest_area(cd_vcpu, gfn_to_gaddr( > > + mfn_to_gfn(d, > > page_to_mfn(d_area->pg))) + > > + offset, > > + PAGE_SIZE - offset, cd_area, NULL); > > +} > > + > > static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu) > > { > > struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu); > > @@ -1709,6 +1727,16 @@ static int copy_vcpu_settings(struct domain *cd, > > const struct domain *d) > > return ret; > > } > > > > + /* Same for the (physically registered) runstate and time info > > areas. */ > > + ret = copy_guest_area(&cd_vcpu->runstate_guest_area, > > + &d_vcpu->runstate_guest_area, cd_vcpu, d); > > + if ( ret ) > > + return ret; > > + ret = copy_guest_area(&cd_vcpu->arch.time_guest_area, > > + &d_vcpu->arch.time_guest_area, cd_vcpu, d); > > + if ( ret ) > > + return ret; > > + > > ret = copy_vpmu(d_vcpu, cd_vcpu); > > if ( ret ) > > return ret; > > @@ -1950,7 +1978,10 @@ int mem_sharing_fork_reset(struct domain *d, bool > > reset_state, > > > > state: > > if ( reset_state ) > > + { > > rc = copy_settings(d, pd); > > + /* TBD: What to do here with -ERESTART? */ > > There is no situation where we get an -ERESTART here currently. Is > map_guest_area expected to run into situations where it fails with > that rc?
Yes, there's a spin_trylock() call that will result in map_guest_area() returning -ERESTART. > If yes we might need a lock in place so we can block until it > can succeed. I'm not sure whether returning -ERESTART can actually happen in map_guest_area() for the fork case: the child domain is still paused at this point, so there can't be concurrent guest hypercalls that would also cause the domain hypercall_deadlock_mutex to be acquired. The comment was added by Jan, so I cannot be certain about the intention, neither I would like to misinterpret his words. My understanding is that future uses of copy_settings() might indeed need to report -ERESTART, and that it would need to be propagated for proper hypercall continuations at some point. Thanks, Roger.
