Re: [XEN PATCH v2 2/5] xen: export get_free_port

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 27/01/2022 01:50, Stefano Stabellini wrote:
> On Wed, 26 Jan 2022, Julien Grall wrote:
> > On 26/01/2022 07:30, Jan Beulich wrote:
> > > On 26.01.2022 02:03, Stefano Stabellini wrote:
> > > > Are you guys OK with something like this?
> > > With proper proof that this isn't going to regress anything else, maybe.
> > That's why I sugested to also gate with system_state < SYS_STATE_boot so we
> > reduce the potential regression (the use of hypercall should be limited at
> > boot).
> >
> > FWIW, there was a thread [1] to discuss the same issue as Stefano is facing
> > (but in the context of Live-Update).
> >
> > > But ...
> > >
> > > > --- a/xen/include/xsm/dummy.h
> > > > +++ b/xen/include/xsm/dummy.h
> > > > @@ -92,7 +92,9 @@ static always_inline int xsm_default_action(
> > > >                return 0;
> > > >            /* fall through */
> > > >        case XSM_PRIV:
> > > > -        if ( is_control_domain(src) )
> > > > +        if ( is_control_domain(src) ||
> > > > +             src->domain_id == DOMID_IDLE ||
> > > > +             src->domain_id == DOMID_XEN )
> > > >                return 0;
> > > ... my first question would be under what circumstances you might observe
> > > DOMID_XEN here and hence why this check is there.
> For simplicity I'll answer all the points here.
>
> I added both DOMID_IDLE and DOMID_XEN because I thought it "made sense",
> but there is no need for DOMID_XEN. We only need DOMID_IDLE. Also adding
> a system_state <= SYS_STATE_boot is fine (but it needs to be <= instead
> of <). The patch appended below works without issues.
>
> Alternatively, I am also quite happy with Jan's suggestion of passing an
> extra parameter to evtchn_alloc_unbound, it could be:
>
> int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool disable_xsm);
>
> So that XSM is enabled by default.
>
> Adding the bool parameter to evtchn_alloc_unbound has the advantage of
> having only a very minor impact.
We will likely need similar approach for other hypercalls in the future 
if we need to call them from Xen context (e.g. when restoring domain 
state during Live-Update).
So my preference would be to directly go with modifying the 
xsm_default_action().
> But either option is totally fine by
> me, just let me know your preference.
>
>
>
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index b024119896..01996bd9d8 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -94,6 +94,8 @@ static always_inline int xsm_default_action(
>       case XSM_PRIV:
>           if ( is_control_domain(src) )
>               return 0;
> +        if ( system_state <= SYS_STATE_boot && src->domain_id == DOMID_IDLE )
I would surround this with unlikely and possibly prevent speculation (Jan?).

Cheers,

--
Julien Grall