Re: [Xen-devel] [PATCH v3] xen: introduce VCPUOP_register_runstate_phys_memory_area hypercall

[Andrii Anisov <andrii.anisov@xxxxxxxxx>]

Hello Jan,

On 11.06.19 12:10, Jan Beulich wrote:
> > > Except when there's no need for KPTI in the guest in the first place,
> > > as is the case for x86-64 PV guests. I think this is worthwhile clarifying.
> > I am not sure what is your point here. At least on Arm, using virtual address is
> > not safe at all (whether KPTI is used or not). A guest can genuinely decides to
> > shatter the mapping where the virtual address is. On Arm, this require to use
> > the break-before-make sequence. It means the translation VA -> PA may fail is
> > you happen to do it while the guest is using the sequence.
> >
> > Some of the intermittent issues I have seen on the Arndale in the past [1] might
> > be related to using virtual address. I am not 100% sure because even if the
> > debug, the error does not make sense. But this is the most plausible reason for
> > the failure.
> All fine, but Arm-specific. The point of my comment was to ask to call
> out that there is one environment (x86-64 PV) where this KPTI
> discussion is entirely inapplicable.
I admit that x86 specifics are quite unclear to me so clarifications and 
corrections in that domain are desirable.

> > > > @@ -35,8 +37,16 @@ arch_compat_vcpu_op(
> > > >                 !compat_handle_okay(area.addr.h, 1) )
> > > >                break;
> > > >    
> > > > +        while( xchg(&v->runstate_in_use, 1) == 0);
> > > At the very least such loops want a cpu_relax() in their bodies.
> > > But this being on a hypercall path - are there theoretical guarantees
> > > that a guest can't abuse this to lock up a CPU?
> > Hmmm, I suggested this but it looks like a guest may call the hypercall multiple
> > time from different vCPU. So this could be a way to delay work on the CPU.
Julien, I'm not sure I understand how work on (p?)CPU could be delayed. We are 
here with interrupts enabled, so here guest would just spend his own vcpu time 
budget. On the runstate update there is a kind-of trylock, so we would not hang 
there either. So what the problem?

> > I wanted to make the context switch mostly lockless and therefore avoiding to
> > introduce a spinlock.>
> Well, constructs like the above are trying to mimic a spinlock
> without actually using a spinlock. There are extremely rare
> situation in which this may indeed be warranted, but here it
> falls in the common "makes things worse overall" bucket, I
> think. To not unduly penalize the actual update paths, I think
> using a r/w lock would be appropriate here.
Firstly I did not think r/w lock specifics are needed here. We have only one 
reader path - runstate update on vcpu scheduling. And this path can have only 
one instance at the time.
But it seems to be more efficient than the spinlock for the case we are not 
locking.

--
Sincerely,
Andrii Anisov.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel