From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vijay Kilari Subject: Re: [PATCH v5 12/22] xen/arm: ITS: Add GICR register emulation Date: Fri, 31 Jul 2015 14:38:40 +0530 Message-ID: References: <1437995524-19772-1-git-send-email-vijay.kilari@gmail.com> <1437995524-19772-13-git-send-email-vijay.kilari@gmail.com> <55BA591C.6020004@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55BA591C.6020004@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 Thu, Jul 30, 2015 at 10:34 PM, Julien Grall wrote: > Hi Vijay, > > On 27/07/15 12:11, vijay.kilari@gmail.com wrote: >> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > > [..] > >> +static int gicv3_dist_supports_lpis(void) >> +{ >> + return readl_relaxed(GICD + GICD_TYPER) & GICD_TYPER_LPIS_SUPPORTED; >> +} >> + >> static int __cpuinit gicv3_cpu_init(void) >> { >> int i; >> @@ -1274,6 +1279,11 @@ static int __init gicv3_init(void) >> >> spin_lock(&gicv3.lock); >> >> + if ( gicv3_dist_supports_lpis() ) >> + gicv3_info.lpi_supported = 1; >> + else >> + gicv3_info.lpi_supported = 0; >> + > > You will avoid 3 lines if you do: > > gicv3_info.lpi_supported = !!gicv3_dist_supports_lpis(); > This will change in patch #17 where we do check for its_probe() to be succesful to set gicv3_info.lpi_supported >> gicv3_dist_init(); >> res = gicv3_cpu_init(); >> gicv3_hyp_init(); >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index 1757193..af8a34b 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -67,6 +67,11 @@ unsigned int gic_number_lines(void) >> return gic_hw_ops->info->nr_lines; >> } >> >> +bool_t gic_lpi_supported(void) >> +{ >> + return gic_hw_ops->info->lpi_supported; >> +} >> + >> void gic_save_state(struct vcpu *v) >> { >> ASSERT(!local_irq_is_enabled()); >> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c >> index 1c7d9b6..4afb62b 100644 >> --- a/xen/arch/arm/vgic-v3-its.c >> +++ b/xen/arch/arm/vgic-v3-its.c > > Can you explain why the emulation of the LPI property table has to be > done in the vITS code? > > Overall, there is nothing vITS specific in this code and all the > functions you've introduced within this file are called only by the > vgic-v3 code. yes, it is called from vgic-v3 code because it is emulating GICR_PROP/PEND registers. This is emulating LPI property table. Hence all the LPI handling code is kept in vits file. GICR_PROP/PEND is defined only for LPI case. > >> @@ -28,6 +28,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -76,6 +77,34 @@ static inline uint32_t vits_get_max_collections(struct domain *d) >> return (d->max_vcpus + 1); >> } >> >> +static void vits_disable_lpi(struct vcpu *v, uint32_t vlpi) >> +{ >> + struct pending_irq *p; >> + >> + p = irq_to_pending(v, vlpi); >> + clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); >> + gic_remove_from_queues(v, vlpi); >> +} >> + >> +static void vits_enable_lpi(struct vcpu *v, uint32_t vlpi, uint8_t priority) >> +{ >> + struct pending_irq *p; >> + unsigned long flags; >> + >> + p = irq_to_pending(v, vlpi); >> + >> + set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); >> + >> + spin_lock_irqsave(&v->arch.vgic.lock, flags); >> + >> + /*XXX: raise on right vcpu */ > > As said on the previous versions, I think there will be locking issue > given that pending_irq structure is protected by the vCPU where the IRQ > is locked. Can you please explain in detail why there is a locking issue. I remember this locking mechanism is coming from enable_irqs() as we follow same infrastructure to inject/process LPIs > > If you think it's not the case please explain why but don't leave the > question unanswered. > [...] >> >> + >> +int vits_map_lpi_prop(struct domain *d) >> +{ >> + struct vgic_its *vits = d->arch.vgic.vits; >> + paddr_t gaddr, addr; >> + unsigned long mfn; >> + uint32_t lpi_size, id_bits; >> + int i; >> + >> + gaddr = vits->propbase & MASK_4K; > > The physical address is only bits [47:12]. The uppers are either RES0 or > used for the OuterCache. Can you please be more explicit. What is the issue here? > >> + id_bits = ((vits->propbase & GICR_PROPBASER_IDBITS_MASK) + 1); >> + >> + if ( id_bits > d->arch.vgic.id_bits ) >> + id_bits = d->arch.vgic.id_bits; > > As said on v4, you are allowing the possibility to have a smaller > property table than the effective number of LPIs. > > An ASSERT in vits_get_priority (patch #16) doesn't ensure the validity > of the size of the property table provided by the guest. This will > surely crash Xen in debug mode, and who knows what will happen in > production mode. lpi_size is calculated based on id_bits. If it is smaller, the lpi_size will be smaller where only size of lpi_size is considered. >> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >> index 683e3cc..a466a8f 100644 >> --- a/xen/arch/arm/vgic-v3.c >> +++ b/xen/arch/arm/vgic-v3.c >> @@ -29,6 +29,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> #include >> >> /* GICD_PIDRn register values for ARM implementations */ >> @@ -109,29 +111,47 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, >> struct hsr_dabt dabt = info->dabt; >> struct cpu_user_regs *regs = guest_cpu_user_regs(); >> register_t *r = select_user_reg(regs, dabt.reg); >> - uint64_t aff; >> + uint64_t aff, val; >> >> switch ( gicr_reg ) >> { >> case GICR_CTLR: >> - /* We have not implemented LPI's, read zero */ >> - goto read_as_zero_32; >> + if ( dabt.size != DABT_WORD ) goto bad_width; >> + vgic_lock(v); >> + *r = v->domain->arch.vgic.gicr_ctlr; >> + vgic_unlock(v); >> + return 1; >> case GICR_IIDR: >> if ( dabt.size != DABT_WORD ) goto bad_width; >> *r = GICV3_GICR_IIDR_VAL; >> return 1; >> case GICR_TYPER: >> - if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; >> - /* TBD: Update processor id in [23:8] when ITS support is added */ >> + case GICR_TYPER + 4: > > Why did you introduce it only in v5? This change is not related to the > LPI but a correct implementation of access size on GICv3 register. > > Overall, I think this should be done in a separate patch for all the > registers and not only this one. It would make the code change less > complicate to read. > > Please fix the write version of TYPER which happen to be 64 bits only too. I added it because, after updating my Linux kernel driver, I see that it is making 32-bit access to TYPER register static bool gic_rdists_supports_plpis(void) { return !!(readl_relaxed(gic_data_rdist_rd_base() + GICR_TYPER) & GICR_TYPER_PLPIS); } But spec doesn't say it supports 32-bit access. Regards Vijay