Re: [PATCH for-4.6 2/4] xen/arm: vgic: Keep track of vIRQ used by a domain

[Ian Campbell <Ian.Campbell@citrix.com>]

On Tue, 2015-01-13 at 16:27 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 13/01/15 15:51, Ian Campbell wrote:
> > On Fri, 2014-12-12 at 14:43 +0000, Julien Grall wrote:
> >> While it's easy to know which hardware IRQ is assigned to a domain, there
> >> is no way to know which IRQ is emulated by Xen for a specific domain.
> > 
> > It seems you are tracking all valid interrupts, including hardware ones,
> > not just those for emulated devices? Perhaps rather than emulated you
> > meant "allocated to the guest" or "routed" or something?
> > 
> >> Introduce a bitmap to keep track of every vIRQ used by a domain. This
> >> will be used later to find free vIRQ for interrupt device assignment and
> >> emulated interrupt.
> > 
> > Actually, don't you implement the alloc/free of vIRQs here too?
> 
> Yes.
> 
> > Is there a usecase for tracking SPIs in this way, or would tracking PPIs
> > only be sufficient?
> 
> We need to track everything for interrupt assignment to a guest/dom0. So
> if the guest ask for a free vIRQ we can give it directly.

Makes sense.

In that case you 0/4 mail doesn't fully describe the use case for the
series, since it talks about the dom0 PPI only.
> > 
> >> +    if ( !d->arch.vgic.allocated_irqs )
> >> +        return -ENOMEM;
> >> +
> >> +    /* vIRQ0-15 (SGIs) are reserved */
> >> +    for (i = 0; i <= 15; i++)
> >> +        set_bit(i, d->arch.vgic.allocated_irqs);
> >> +
> >>      return 0;
> >>  }
> >>  
> >> @@ -119,6 +130,7 @@ void domain_vgic_free(struct domain *d)
> >>  {
> >>      xfree(d->arch.vgic.shared_irqs);
> >>      xfree(d->arch.vgic.pending_irqs);
> >> +    xfree(d->arch.vgic.allocated_irqs);
> >>  }
> >>  
> >>  int vcpu_vgic_init(struct vcpu *v)
> >> @@ -441,6 +453,70 @@ int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
> >>      return v->domain->arch.vgic.handler->emulate_sysreg(regs, hsr);
> >>  }
> >>  
> >> +bool_t vgic_reserve_virq(struct domain *d, unsigned int virq)
> >> +{
> >> +    bool_t reserved;
> >> +
> >> +    if ( virq >= vgic_num_irqs(d) )
> >> +        return 0;
> > 
> > EINVAL?
> 
> vgic_reserve_irq returns a boolean:

Please use true/false then.

In Xen we have xen/stdbool.h which differs from normal stdboot.h. I'm
not sure what the rules are for use.

> 	0 => not reserved
> 	1 => reserved
> 
> I don't see why we should return an int in this case, as the caller
> should know how to use it.

It's slightly more conventional to return error codes, but I guess I
don't mind much.

> >> @@ -49,6 +49,21 @@ int domain_vtimer_init(struct domain *d)
> >>  {
> >>      d->arch.phys_timer_base.offset = NOW();
> >>      d->arch.virt_timer_base.offset = READ_SYSREG64(CNTPCT_EL0);
> >> +
> >> +    /* At this stage vgic_reserve_virq can't fail */
> >> +    if ( is_hardware_domain(d) )
> >> +    {
> >> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_SECURE_PPI)));
> >> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_PHYS_NONSECURE_PPI)));
> >> +        BUG_ON(!vgic_reserve_virq(d, timer_get_irq(TIMER_VIRT_PPI)));
> >> +    }
> >> +    else
> >> +    {
> >> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_S_PPI));
> >> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_PHYS_NS_PPI));
> >> +        BUG_ON(!vgic_reserve_virq(d, GUEST_TIMER_VIRT_PPI));
> > 
> > Although BUG_ON is not conditional on $debug I think we still should
> > avoid side effects in the condition.
> 
> I know, but this should never fail as it called during on domain
> construction. If so we may have some other issue later if we decide to
> assign PPI to a guest.
> 
> I would prefer to keep the BUG_ON here

I'm not objecting the the BUG_ON itself but to the fact that the
condition has a side effect. Please use:
        if (!do_something())
        	BUG()
instead to avoid this.

Ian.