From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 06/17] xen/arm: ITS: Add virtual ITS driver Date: Wed, 15 Jul 2015 14:17:24 +0200 Message-ID: <55A64F54.30707@citrix.com> 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"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436514172-3263-7-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-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 K , manish.jaggi@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 10/07/2015 09:42, vijay.kilari@gmail.com wrote: > +static int vits_entry(struct domain *d, paddr_t entry, void *addr, > + uint32_t size, bool_t set) > +{ > + struct page_info *page; > + uint64_t offset; > + p2m_type_t p2mt; > + void *p; > + > + page = get_page_from_gfn(d, paddr_to_pfn(entry), &p2mt, P2M_ALLOC); > + if ( !page ) > + { > + dprintk(XENLOG_G_ERR, "%pv: vITS: Failed to get table entry\n", > + current); The function vits_entry will be used when an LPI is injected to the guest. At that time, current may not be a VCPU of the domain d. Therefore, this log will be very confusing if an error occur. I would prefer if you only print the domain id using d. Furthermore, dprintk is a nop on non-debug build. You may want to use printk here. Those comments are valid in every dprintk of this patch. [..] > +/* 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), set); > +} > + > +int vits_set_vdevice_entry(struct domain *d, uint32_t devid, > + struct vdevice_table *entry) > +{ > + return vits_vdevice_entry(d, devid, entry, 1); > +} > + > +int vits_get_vdevice_entry(struct domain *d, uint32_t devid, > + struct vdevice_table *entry) > +{ > + return vits_vdevice_entry(d, devid, entry, 0); > +} While I can understand that you export vits_get_vdevice_entry, it's used in the LPI injection. I don't understand for vits_set_vdevice_entry. > + > +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", s/dev/vdevid/ > + current, devid); > + return -EINVAL; > + } > + > + /* dt_entry is validated when read */ > + 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); > +} > + > +int vits_set_vitt_entry(struct domain *d, uint32_t devid, > + uint32_t event, struct vitt *entry) > +{ > + return vits_vitt_entry(d, devid, event, entry, 1); > +} > + > +int vits_get_vitt_entry(struct domain *d, uint32_t devid, > + uint32_t event, struct vitt *entry) > +{ > + return vits_vitt_entry(d, devid, event, entry, 0); > +} Same remark as vits_*_device_entry. [..] > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index f1a087e..67e4695 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -115,6 +115,10 @@ struct arch_domain > #endif > } vgic; > > +#ifdef CONFIG_ARM_64 > + struct vgic_its *vits; > +#endif > + I would prefer to see this field part of the vgic structure (see just above). There is already a #ifdef for GICv3 stuff so it will avoid 2 new lines. > struct vuart { > #define VUART_BUF_SIZE 128 > char *buf; > diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h > index e8d244f..d21aefe 100644 > --- a/xen/include/asm-arm/gic-its.h > +++ b/xen/include/asm-arm/gic-its.h > @@ -31,7 +31,20 @@ struct its_collection { > u16 col_id; > }; > > -/* ITS command structure */ > +/* > + * Per domain virtual ITS structure. > + */ > +struct vgic_its > +{ > + /* vITT device table ipa */ > + paddr_t dt_ipa; > + /* vITT device table size */ > + uint64_t dt_size; > + /* Radix-tree root of devices attached to this domain */ > + struct rb_root dev_root; > +}; > + > +/* ITS command structures */ Please fix the typo in the patch where you introduced it. Not after. Regards, -- Julien Grall