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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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.

Regards,

[1] http://www.gossamer-threads.com/lists/xen/devel/361849

-- 
Julien Grall

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