From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vijay Kilari Subject: Re: [PATCH v5 07/22] xen/arm: ITS: Add virtual ITS driver Date: Fri, 31 Jul 2015 12:19:48 +0530 Message-ID: References: <1437995524-19772-1-git-send-email-vijay.kilari@gmail.com> <1437995524-19772-8-git-send-email-vijay.kilari@gmail.com> <55B7B820.5020708@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55B7B820.5020708@citrix.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: Julien Grall Cc: Ian Campbell , Stefano Stabellini , Prasun Kapoor , manish.jaggi@caviumnetworks.com, Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini , Vijaya Kumar K List-Id: xen-devel@lists.xenproject.org On Tue, Jul 28, 2015 at 10:43 PM, Julien Grall wrote: > Hi Vijay, > > On 27/07/15 12:11, vijay.kilari@gmail.com wrote: >> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >> new file mode 100644 >> index 0000000..60f8332 >> --- /dev/null >> +++ b/xen/arch/arm/vgic-v3-its.c >> +static int vits_access_guest_table(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 ) >> + { >> + printk(XENLOG_G_ERR "d%"PRId32": vITS: Failed to get table entry\n", > > A domain id is encoded on 16 bits and not 32 bits. Furthermore it's an > unsigned. Here you will print signed. > > My remark will be the same for all similar use within 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; >> + struct vgic_its *vits = d->arch.vgic.vits; > > const struct > > [..] > >> +int vits_set_vdevice_entry(struct domain *d, uint32_t devid, >> + struct vdevice_table *entry) >> +{ >> + return vits_vdevice_entry(d, devid, entry, 1); >> +} > > The prototype is not specified in the header. So either you need to add > the prototype, if used outside the file, or set static. > > [..] > >> +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) ) >> + { >> + printk(XENLOG_G_ERR >> + "d%"PRId32": vITS: Fail to get vdevice for vdev 0x%"PRIx32"\n", > > s/vdev/vdevid/ I think, to manage within 80 char, I used just "vdev" > >> + d->domain_id, devid); >> + return -EINVAL; >> + } >> + >> + /* dt_entry is validated in vits_get_vdevice_entry */ > > s/is validated/has been validated/ > > [..] > >> +int vits_set_vitt_entry(struct domain *d, uint32_t devid, >> + uint32_t event, struct vitt *entry) > > Same remark as vits_set_vdevice_entry. I have made non-static for compilation purpose. I will try to introduce this in the patch where it is used. But it is more logical to have this in this patch. Anyway I forget to make it static in later patches > > Regards, > > -- > Julien Grall