Re: [Xen-devel] [PATCH RFC] xen: arm: context switch vtimer PPI state.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2015-01-27 at 13:16 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 26/01/15 15:55, Ian Campbell wrote:
> > ... instead of artificially masking the timer interrupt in the timer
> > state and relying on the guest to unmask (which it isn't required to
> > do per the h/w spec, although Linux does)
> > 
> > To do this introduce the concept of routing a PPI to the currently
> > running VCPU. For such interrupts irq_get_domain returns NULL.
> 
> > Then we make use of the GICD I[SC]ACTIVER registers to save and
> > restore the active state of the interrupt, which prevents the nested
> > invocations which the current masking works around.
> > 
> > For edge triggered interrupts we also need to context switch the
> > pending bit via I[SC]PENDR. Note that for level triggered interrupts
> > SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
> > state changes), therefore we do not want to context switch the pending
> > state for level PPIs -- instead we rely on the context switch of the
> > peripheral to restore the correct level.
> > 
> > RFC Only:
> >  - Not implemented for GIC v3 yet.
> >  - Lightly tested with hackbench on systems with level and edge
> >    triggered vtimer PPI.
> >  - Is irq_get_domain == NULL to indicate route-to-current-vcpu the
> >    best idea? Any alternatives?
> 
> I have introduced an irq_guest structure in my platform device
> passthrough series (https://patches.linaro.org/43012/).
> 
> Maybe you could extend it to avoid things like irq_get_domain == NULL.

I'll have a look.

> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 31fb81a..9074aca 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -156,6 +156,45 @@ static void gicv2_save_state(struct vcpu *v)
> >      writel_gich(0, GICH_HCR);
> >  }
> >  
> > +static void gicv2_save_and_mask_hwppi(struct vcpu *v, const unsigned virq,
> > +                                      struct hwppi_state *s)
> > +{
> > +    struct pending_irq *p = irq_to_pending(v, virq);
> > +    const unsigned int offs = virq / 32;
> > +    const unsigned int mask = (1u << (virq % 32));
> > +    const unsigned int pendingr = readl_gicd(GICD_ISPENDR + offs*4);
> > +    const unsigned int activer = readl_gicd(GICD_ISACTIVER + offs*4);
> > +    const unsigned int enabler = readl_gicd(GICD_ISENABLER + offs*4);
> > +    const bool is_edge = !!(p->desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> 
> "For each supported PPI, it is IMPLEMENTATION DEFINED whether software
> can program the corresponding Int_config field."
> 
> So I would not rely on arch.type as it may not be in sync with the register.
> 
> It will be more problematic with the upcoming patch to let the guest
> configure ICFGR0.

If anything is reprogramming ICFGR0 and not reflecting that in the
corresponding desc then it is buggy IMHO. Either arch.type should be
always valid or it should be removed.

For ease of implementation I think we should probably refuse to allow
any ppi used via this scheme to be reprogrammed, i.e. Xen should choose
at start of day (based on DTB, h/w capability) and just force it upon
the guest.

Context switching icfg and handling the subtleties of flipping mode just
sounds horrible.

> > +    if ( s->enabled )
> > +    {
> > +        writel_gicd(mask, GICD_ICENABLER + offs*4);
> > +        set_bit(_IRQ_DISABLED, &p->desc->status);
> 
> I would prefer if you use gicv2_irq_disable rather than open coding.

I'd like to hear from Stefano about whether this is the correct thing to
do in general before changing this. If context switching the status bits
is not required/wrong or there is some preferable way etc.

More generally the way pending_irq and the irq descriptor are related
after this patch needs some careful though IMHO.

> > @@ -125,11 +137,15 @@ void gic_route_irq_to_xen(struct irq_desc *desc, 
> > const cpumask_t *cpu_mask,
> >  
> >  /* Program the GIC to route an interrupt to a guest
> >   *   - desc.lock must be held
> > + *   - d may be NULL, in which case interrupt *must* be a PPI and is 
> > routed to
> > + *     the vcpu currently running when that PPI fires. In this case the 
> > code
> > + *     responsible for the related hardware must save and restore the PPI 
> > with
> > + *     gic_save_and_mask_hwppi/gic_restore_hwppi.
> > + *   - if d is non-NULL then the interrupt *must* be an SPI.
> >   */
> 
> the d == NULL sounds more an hackish way to specify this IRQ is routed
> to any guest. I would prefer if you introduce a separate function and
> duplicate the relevant bits.

That is route_irq_to_current_vcpu vs route_irq_to_guest which already
exist as the API the callers actually use.

> > +    ASSERT( d || ( irq >=16 && irq < 32 ) );
> > +
> 
> Please no ASSERT in this function. This will be soon expose to the guest
> (see my device passthrough series).

Then you should add range checking on the inputs at the hypercall level,
since irrespective of whether the ASSERT is there or that condition must
be true for things to function correctly.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel