Re: [PATCH v3 2/2] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/17/26 3:26 PM, Jan Beulich wrote:
> On 03.06.2026 16:25, Oleksii Kurochko wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -320,9 +320,9 @@ void vcpu_info_reset(struct vcpu *v)
> >       struct domain *d = v->domain;
> >   
> >       v->vcpu_info_area.map =
> > -        ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
> > -         ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> > -         : &dummy_vcpu_info);
> > +        IS_ENABLED(CONFIG_HAS_SHARED_INFO) && v->vcpu_id < XEN_LEGACY_MAX_VCPUS
> > +        ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> > +        : &dummy_vcpu_info;
> >   }
> While the change here is likely okay, it points at possible further omissions.
> You've dealt with all uses of shared_info(), but you've left alone all uses of
> vcpu_info() (and __vcpu_info()). Reads are presumably okay, but writes to
> dummy_vcpu_info open a side channel for possible info leaks. Looking over uses
> in common code, no code changes may be needed; extending the description may
> be all that's wanted here.
Isn't there already a side channel that could allow leaks, even without 
this change? The change here just made it worsen because now info leak 
will happen for all vCPUs when  CONFIG_HAS_SHARED_INFO=n.
I will add to the description the following:
```
With CONFIG_HAS_SHARED_INFO=n all vCPUs fall back to the global
dummy_vcpu_info, so writes through vcpu_info() could leak data between
vCPUs.  Reviewing the write paths in common code: the write in
map_guest_area() stores the constant ~0 so nothing serious will happen 
if it will be leaked; the event_2l.c paths are unreachable because the 
preceding shared_info() call would trap first; the write in 
vcpu_info_populate() targets the new mapping buffer, not 
dummy_vcpu_info; all remaining writes are x86 PV-specific for which 
CONFIG_HAS_SHARED_INFO=y.  No code changes are needed.
```

> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -1323,9 +1323,13 @@ int evtchn_reset(struct domain *d, bool resuming)
> >           rc = -EAGAIN;
> >       else if ( d->evtchn_fifo )
> >       {
> > -        /* Switching back to 2-level ABI. */
> >           evtchn_fifo_destroy(d);
> > -        evtchn_2l_init(d);
> > +
> > +        if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
> > +            /* Switching back to 2-level ABI. */
> > +            evtchn_2l_init(d);
> > +        else
> > +            evtchn_fifo_init_ops(d);
> >       }
> The "else" part isn't needed, is it? evtchn_fifo_destroy() doesn't undo
> what ...
Agree, it isn't really needed. I will drop "else" part.

> > @@ -1624,7 +1628,11 @@ void evtchn_check_pollers(struct domain *d, unsigned int 
> > port)
> >   
> >   int evtchn_init(struct domain *d, unsigned int max_port)
> >   {
> > -    evtchn_2l_init(d);
> > +    if ( IS_ENABLED(CONFIG_HAS_SHARED_INFO) )
> > +        evtchn_2l_init(d);
> > +    else
> > +        evtchn_fifo_init_ops(d);
> ... was done here.
>
> > --- a/xen/include/xen/shared.h
> > +++ b/xen/include/xen/shared.h
> > @@ -43,7 +43,14 @@ typedef struct vcpu_info vcpu_info_t;
> >   
> >   extern vcpu_info_t dummy_vcpu_info;
> >   
> > +#ifdef CONFIG_HAS_SHARED_INFO
> >   #define shared_info(d, field)      __shared_info(d, (d)->shared_info, field)
> > +#else
> > +void * shared_info_absent(void);
> Nit: Stray blank after '*'.
>
> > +#define shared_info(d, field) \
> > +    (*(typeof(__shared_info(d, (d)->shared_info, field)) 
> > *)shared_info_absent())
> How about the simpler
>
> extern struct shared_info *shared_info_absent;
> #define shared_info(d, field) (shared_info_absent->field)
>
> ?
It would be better. I will apply that.

Thanks.

~ Oleksii