Re: [PATCH RFC 04/10] domain: update GADDR based runstate guest area

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 19/10/2022 08:41, Jan Beulich wrote:
> Before adding a new vCPU operation to register the runstate area by
> guest-physical address, add code to actually keep such areas up-to-date.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> RFC: Pages aren't marked dirty when written to (matching the handling of
>       space mapped by map_vcpu_info() afaict), on the basis that the
>       registrations are lost anyway across migration. 
So I agree for the existing migration. But I wonder whether we would 
need to dirty those pages if we live-migrated a guest without its help 
(IOW the registrations would still be present).
Anyway, nothing to worry about yet as this is not supported upstream.

> Plus the contents
>       of the areas in question have to be deemed volatile in the first
>       place (so saving a "most recent" value is pretty meaningless even
>       for e.g. snapshotting).
>
> RFC: Can we perhaps avoid the VM-assist conditionals, assuming the more
>       modern behavior to apply uniformly for gaddr-based registrations?
It is not clear why someone would want to use the old behavior with the 
new gaddr-based registrations. So I would say yes.
> RFC: HVM guests (on x86) can change bitness and hence layout (and size!
>       and alignment) of the runstate area. I don't think it is an option
>       to require 32-bit code to pass a range such that even the 64-bit
>       layout wouldn't cross a page boundary (and be suitably aligned). I
>       also don't see any other good solution, so for now a crude approach
>       with an extra boolean is used (using has_32bit_shinfo() isn't race
>       free and could hence lead to overrunning the mapped space).
I think the extra check for 32-bit code to pass the check for 64-bit 
layout would be better.
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1599,14 +1599,55 @@ bool update_runstate_area(struct vcpu *v
>       struct guest_memory_policy policy = { };
>       void __user *guest_handle = NULL;
>       struct vcpu_runstate_info runstate;
> +    struct vcpu_runstate_info *map = v->runstate_guest_area.map;
> +
> +    memcpy(&runstate, &v->runstate, sizeof(runstate));
> +
> +    if ( map )
> +    {
> +        uint64_t *pset = NULL;
> +#ifdef CONFIG_COMPAT
> +        struct compat_vcpu_runstate_info *cmap = NULL;
> +
> +        if ( v->runstate_guest_area_compat )
> +            cmap = (void *)map;
> +#endif
> +
> +        if ( VM_ASSIST(v->domain, runstate_update_flag) )
> +        {
> +#ifdef CONFIG_COMPAT
> +            if ( cmap )
> +                pset = &cmap->state_entry_time;
> +            else
> +#endif
> +                pset = &map->state_entry_time;
> +            runstate.state_entry_time |= XEN_RUNSTATE_UPDATE;
> +            write_atomic(pset, runstate.state_entry_time);
> +            smp_wmb();
> +        }
> +
> +#ifdef CONFIG_COMPAT
> +        if ( cmap )
> +            XLAT_vcpu_runstate_info(cmap, &runstate);
> +        else
> +#endif
> +            *map = runstate;
> +
> +        if ( pset )
> +        {
> +            smp_wmb();
> +            runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE;
> +            write_atomic(pset, runstate.state_entry_time);
> +        }
> +
> +        return true;
> +    }
>   
>       if ( guest_handle_is_null(runstate_guest(v)) )
>           return true;
>   
>       update_guest_memory_policy(v, &policy);
>   
> -    memcpy(&runstate, &v->runstate, sizeof(runstate));
> -
>       if ( VM_ASSIST(v->domain, runstate_update_flag) )
>       {
>   #ifdef CONFIG_COMPAT
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -231,6 +231,8 @@ struct vcpu
>   #ifdef CONFIG_COMPAT
>       /* A hypercall is using the compat ABI? */
>       bool             hcall_compat;
> +    /* Physical runstate area registered via compat ABI? */
> +    bool             runstate_guest_area_compat;
>   #endif
>   
>   #ifdef CONFIG_IOREQ_SERVER
Cheers,

--
Julien Grall