Re: [Embedded-pv-devel] Driver domain under Xen

[Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>]

On Fri, Jan 23, 2015 at 6:05 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> On 23/01/15 15:22, Andrii Tseglytskyi wrote:
>> OK. I'm sending you my local changes - 3 pieces of code.
>
> Few comments below.
>
>> Author: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>> Date:   Tue Dec 30 18:38:55 2014 +0200
>>
>>     xen/arm: always use count number to release hardware irq
>>
>>     As soons as hardware interrupts are always mapped 1 to 1
>>     count number plus 32 is always equal to interrupt number which
>>     should be released. In current implementation if we don't receive
>>     any irq which number should be released existing p->irq number will
>>     be equal to 0, and interrupt will be not released properly.
>>     So, use lopp counter + 32 here.
>
> release_guest_irq is taking a vIRQ in parameter and not an IRQ.
>
>>     Change-Id: If89c928ff3ac5b83319d8b2b91439a7d598ed66d
>>     Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>
> See https://patches.linaro.org/42995/
>
>> commit 52db4eacef9c3ab050bba2548196bcaff657bc00
>> Author: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>> Date:   Fri Aug 29 14:39:31 2014 +0300
>>
>>     xen/arm: allow reassigning of hw interrupts to guest domain
>>
>>     Patch allows reassigning of hardware interrupts from dom0 to
>>     other guest domain.
>>
>>     Change-Id: Ie85485830d87b07393cc7fe7e64de5fd5f65ebb8
>>     Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>>
>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>> index ba33571..018a2f6 100644
>> --- a/xen/arch/arm/irq.c
>> +++ b/xen/arch/arm/irq.c
>> @@ -455,12 +455,24 @@ int route_irq_to_guest(struct domain *d,
>> unsigned int virq,
>>              goto out;
>>
>>          if ( test_bit(_IRQ_GUEST, &desc->status) )
>> -            dprintk(XENLOG_G_ERR, "IRQ %u is already used by domain %u\n",
>> -                    irq, ad->domain_id);
>> +        {
>> +            dprintk(XENLOG_G_DEBUG, "IRQ %u is reassigned from domain
>> %u to domain %u\n",
>> +                    irq, ad->domain_id, d->domain_id);
>> +
>> +            clear_bit(_IRQ_DISABLED, &desc->status);
>
> Didn't you mean set_bit? We should not be able to remove an IRQ from
> DOM0 if it's enabled. Very bad thing could happen, such as the IRQ still
> in LRs...
>
> In any case, it clearly wrong to modify &desc->status in this code
> because it's related to the interrupt status in the hardware.
>
>> commit 09f50d7864b249bc4d53cb68c5c409ce7e8560f3
>> Author: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>> Date:   Tue Nov 25 19:27:44 2014 +0200
>>
>>     xen/arm: map pv domain interrupts numbers one to one
>>
>>     Currently driver domain uses hardware interrupts. They
>>     are mapped as SPIs to guest domains. Patch allows to use
>>     one to one mappings between hardware interrupts numbers
>>     and SPIs numbers.
>>
>>     Change-Id: Ie5704c88979724b489cc33431bdf6a78ff03fe8c
>>     Signed-off-by: Andrii Tseglytskyi <andrii.tseglytskyi@xxxxxxxxxxxxxxx>
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 653da31..e50e35d 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -66,7 +66,7 @@ int domain_vgic_init(struct domain *d)
>>
>>      d->arch.vgic.ctlr = 0;
>>
>> -    if ( is_hardware_domain(d) )
>> +    if ( is_hardware_domain(d) || is_pv_domain(d) )
>
> I don't understand this change. ARM only support one kind of domain.
>
> We should never use is_{pv,hvm}_domain on ARM as it doesn't have any
> meaning for this architecture.
>
> Give a look to my new patch series [1], for the interrupt parts it will
> really help you.

Julien,

This is why I didn't want to send you code right now. All these
changes work for us to have 1 to 1 IRQ mapping in driver domain and to
release them correctly in case of domain crash. Before get them
upstreamed I would prefer to think twice on each of them and rework if
needed.

Regards,
Andrii

>
> Regards,
>
> [1] http://www.gossamer-threads.com/lists/xen/devel/361849
>
> --
> Julien Grall



-- 

Andrii Tseglytskyi | Lead engineer
GlobalLogic
www.globallogic.com

_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel