[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2] xen/arm: implement GICD_I[S/C]ACTIVER reads



Hi Stefano,

On 30/03/2020 17:35, Stefano Stabellini wrote:
On Sat, 28 Mar 2020, Julien Grall wrote:
qHi Stefano,

On 27/03/2020 02:34, Stefano Stabellini wrote:
This is a simple implementation of GICD_ICACTIVER / GICD_ISACTIVER
reads. It doesn't take into account the latest state of interrupts on
other vCPUs. Only the current vCPU is up-to-date. A full solution is
not possible because it would require synchronization among all vCPUs,
which would be very expensive in terms or latency.

Your sentence suggests you have number showing that correctly emulating the
registers would be too slow. Mind sharing them?

No, I don't have any numbers. Would you prefer a different wording or a
better explanation? I also realized there is a typo in there (or/of).
Let me start with I think correctness is more important than speed.
So I would have expected your commit message to contain some fact why synchronization is going to be slow and why this is a problem.

To give you a concrete example, the implementation of set/way instructions are really slow (it could take a few seconds depending on the setup). However, this was fine because not implementing them correctly would have a greater impact on the guest (corruption) and they are not used often.

I don't think the performance in our case will be in same order magnitude. It is most likely to be in the range of milliseconds (if not less) which I think is acceptable for emulation (particularly for the vGIC) and the current uses.

So lets take a step back and look how we could implement ISACTIVER/ICACTIVER correctly.

The only thing we need is a snapshot of the interrupts state a given point. I originally thought it would be necessary to use domain_pause() which is quite heavy, but I think we only need the vCPU to enter in Xen and sync the states of the LRs.

Roughly the code would look like (This is not optimized):

    for_each_vcpu(d, v)
    {
       if ( v->is_running )
         smp_call_function(do_nothing(), v->cpu);
    }

    /* Read the state */

A few remarks:
   * The function do_nothing() is basically a NOP.
* I am suggesting to use smp_call_function() rather smp_send_event_check_cpu() is because we need to know when the vCPU has synchronized the LRs. As the function do_nothing() will be call afterwards, then we know the the snapshot of the LRs has been done
   * It would be possible to everything in one vCPU.
   * We can possibly optimize it for the SGIs/PPIs case

This is still not perfect, but I don't think the performance would be that bad.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.