On Tue, Oct 03, 2023 at 04:25:58PM -0400, Tamas K Lengyel wrote: > On Tue, Oct 3, 2023 at 11:07 AM Julien Grall <[email protected]> wrote: > > > > Hi Roger, > > > > On 03/10/2023 15:29, Roger Pau Monné wrote: > > > On Tue, Oct 03, 2023 at 09:53:11AM -0400, Tamas K Lengyel wrote: > > > > Tamas, somehow your e-mails don't show up in my inbox (even if I am > > CCed) or even on lore.kernel.org/xen-devel. It is not even in my SPAM > > folder. > > Thanks, I've switched mailservers, hopefully that resolves the issue. > > > > > >> 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. > > Perhaps turning it into an ASSERT(rc != -ERESTART) is the way to go at > this point. If we run into any cases where it trips we can reason it > out.
In order to avoid possibly returning -ERESTART (which should never be seen by hypercall callers) we might want to convert it to -EBUSY and let the caller pick the pieces. Thanks, Roger.
