From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ijc@xen.org>
Subject: Re: [PATCH v4 06/17] xen/arm: ITS: Add virtual ITS
	driver
Date: Fri, 10 Jul 2015 15:15:42 +0100
Message-ID: <1436537742.10074.50.camel@xen.org>
References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com>
	<1436514172-3263-7-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: <1436514172-3263-7-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
Cc: stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, tim@xen.org, xen-devel@lists.xen.org, julien.grall@citrix.com, stefano.stabellini@citrix.com, manish.jaggi@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org

On Fri, 2015-07-10 at 13:12 +0530, vijay.kilari@gmail.com wrote:
> +static int vits_entry(struct domain *d, paddr_t entry, void *addr,
> +                      uint32_t size, bool_t set)
> +{
> [...]
> +}
> +
> +/* ITS device table helper functions */
> +static int vits_vdevice_entry(struct domain *d, uint32_t dev_id,
> +                              struct vdevice_table *entry, bool_t set)
> +{
> +    uint64_t offset;
> +    paddr_t dt_entry;
> +
> +    BUILD_BUG_ON(sizeof(struct vdevice_table) != 16);
> +
> +    offset = dev_id * sizeof(struct vdevice_table);
> +    if ( offset > d->arch.vits->dt_size )
> +    {
> +        dprintk(XENLOG_G_ERR,
> +                "%pv: vITS: Out of range offset %ld id 0x%x size %ld\n",
> +                current, offset, dev_id, d->arch.vits->dt_size);
> +        return -EINVAL;
> +    }
> +
> +    dt_entry = d->arch.vits->dt_ipa + offset;
> +
> +    return vits_entry(d, dt_entry, (void *)entry,
> +                      sizeof(struct vdevice_table),

Please drop the (void *) cast here, you can pass a "foo *" to a "void *"
without one.

It took me a little while to work out why this was void * before I
realised that vits_entry was a generic helper used for different types
of table. "vits_access_guest_table" to make it clear what it is doing.

> +static int vits_vitt_entry(struct domain *d, uint32_t devid,
> +                           uint32_t event, struct vitt *entry, bool_t set)
> +{
> +    struct vdevice_table dt_entry;
> +    paddr_t vitt_entry;
> +    uint64_t offset;
> +
> +    BUILD_BUG_ON(sizeof(struct vitt) != 8);
> +
> +    if ( vits_get_vdevice_entry(d, devid, &dt_entry) )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: Fail to get vdevice for dev 0x%x\n",
> +                current, devid);
> +        return -EINVAL;
> +    }
> +
> +    /* dt_entry is validated when read */

Please can you say "is validated by vits_get_vdevice_entry" to be very
clear.

> +    offset = event * sizeof(struct vitt);
> +    if ( offset > dt_entry.vitt_size )
> +    {
> +        dprintk(XENLOG_G_ERR, "%pv: vITS: ITT out of range\n", current);
> +        return -EINVAL;
> +    }
> +   
> +    vitt_entry = dt_entry.vitt_ipa + offset;
> +
> +    return vits_entry(d, vitt_entry, (void *)entry,
> +                      sizeof(struct vitt), set);

Same story WRT the cast.

[...]
> @@ -171,11 +184,41 @@ struct its_device {
>      struct rb_node          node;
>  };
>  
> +struct vits_device {
> +    uint32_t vdevid;
> +    struct its_device *its_dev;
> +    struct rb_node node;
> +};
> +

Please add a big comment here:

 /*
  * struct vdevice_table and struct vitt are typically stored in memory
  * which has been provided by the guest out of its own address space 
  * and which remains accessible to the guest.
  *
  * Therefore great care _must_ be taken when accessing an entry in
  * either table to validate the sanity of any values which are used
  */

> +struct vdevice_table {
> +    uint64_t vitt_ipa;
> +    uint32_t vitt_size;
> +    uint32_t padding;
> +};
> +
> +struct vitt {
> +    uint16_t valid:1;
> +    uint16_t pad:15;
> +    uint16_t vcollection;
> +    uint32_t vlpi;
> +};

Ian.