On 08.11.2022 12:38, Roger Pau Monne wrote:
> Like on the Arm side, return -EINVAL when attempting to do a p2m
> operation on dying domains.
>
> The current logic returns 0 and leaves the domctl parameter
> uninitialized for any parameter fetching operations (like the
> GET_ALLOCATION operation), which is not helpful from a toolstack point
> of view, because there's no indication that the data hasn't been
> fetched.
While I can see how the present behavior is problematic when it comes
to consuming supposedly returned data, ...
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -694,9 +694,10 @@ int paging_domctl(struct domain *d, struct
> xen_domctl_shadow_op *sc,
>
> if ( unlikely(d->is_dying) )
> {
> - gdprintk(XENLOG_INFO, "Ignoring paging op on dying domain %u\n",
> + gdprintk(XENLOG_INFO,
> + "Tried to do a paging domctl op on dying domain %u\n",
> d->domain_id);
> - return 0;
> + return -EINVAL;
> }
... going from "success" to "failure" here has a meaningful risk of
regressing callers. It is my understanding that it was deliberate to
mimic success in this case (without meaning to assign "good" or "bad"
to that decision). Can you instead fill the data to be returned in
some simple enough way? I assume a mere memset() isn't going to be
good enough, though (albeit public/domctl.h doesn't explicitly name
any input-only fields, so it may not be necessary to preserve
anything). Maybe zeroing ->mb and ->stats would do?
As a minor remark: _If_ you're changing the printk(), then please
also switch to using %pd.
Jan
