From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v6a 16/17] xen/arm: split vgic driver into
 generic and vgic-v2 driver
Date: Thu, 26 Jun 2014 15:30:57 +0100
Message-ID: <53AC2EA1.5070106@linaro.org>
References: <1403760849-14627-1-git-send-email-vijay.kilari@gmail.com>
	<1403760849-14627-17-git-send-email-vijay.kilari@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1403760849-14627-17-git-send-email-vijay.kilari@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, tim@xen.org, xen-devel@lists.xen.org
Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

Hi Vijay,

On 06/26/2014 06:34 AM, vijay.kilari@gmail.com wrote:

[..]

> +const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {

There was a static on the previous location of this code. You should
keep it.

[..]

> +const static struct vgic_ops vgic_v2_ops = {

static const

[..]

>  int domain_vgic_init(struct domain *d)
>  {
>      int i;
> @@ -75,6 +67,15 @@ int domain_vgic_init(struct domain *d)
>      else
>          d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
>  
> +    switch ( gic_hw_version() )
> +    {
> +    case GIC_V2:
> +        vgic_v2_init(d);

vgic_v2_init is returning an int to indicate if initialization has
succeeded or not. Please check the return to see if there is an error.

[..]

> -static int vgic_to_sgi(struct vcpu *v, register_t sgir)
> +/* TODO: unsigned long is used to fit vcpu_mask. Change to cpu_mask */
> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq,
> +                unsigned long vcpu_mask)
>  {

[..]

> +    case SGI_TARGET_OTHERS:

[..]

> +    case SGI_TARGET_SELF:
> +        set_bit(current->vcpu_id, &vcpu_mask);

I already said it on V4 and V5 and don't see any change here...

For SGI_TARGET_{OTHERS,SELF}, you can't assume that vcpu_mask will be
equal to 0...

it comes directly from GICD_SIGR. Please make sure to handle it or the
guest could send SGI to the wrong VCPUs.

[..]

> +struct vgic_ops {
> +    /* Initialize vGIC */
> +    int (*vcpu_init)(struct vcpu *v);
> +    /* Domain specific initialization of vGIC */
> +    int (*domain_init)(struct domain *d);
> +    /* SGI handler of vGIC */
> +    int (*send_sgi)(struct vcpu *v, register_t sgir);

You've introduce vgic_send_sgi here, I'm not sure to understand why...
Can you give a bit more input?

> +};
> +
> +extern enum gic_sgi_mode irqmode;
> +

Hu? What is used for?

Regards

-- 
Julien Grall