From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 11/17] xen/arm: ITS: Add GICR register emulation Date: Fri, 10 Jul 2015 16:10:08 +0100 Message-ID: <1436541008.10074.84.camel@citrix.com> References: <1436514172-3263-1-git-send-email-vijay.kilari@gmail.com> <1436514172-3263-12-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1436514172-3263-12-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 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: > [...] > +int vits_unmap_lpi_prop(struct vcpu *v) Why is this function called "unmap"? > + /* Register mmio handlers for this region */ > + register_mmio_handler(v->domain, &vgic_gits_lpi_mmio_handler, > + maddr, lpi_size); Something, somewhere, must ensure that we never register this MMIO handler for any guest which does not have a VITS associated with it. As it stands after this series I believe that means that the GITS_* register handling, the GITS LPI MMIO region and the GITS_TRANSLATER mapping should be setup for dom0 if-and-only-if a physical ITS is present in the system. In particular they should never, as it stands today, be mapped for a domU. I am unable to spot the code which arranges all that. There is some if's in the GICR_* handlers, but I don't think that covers everything. Obviously this will change with the addition of PCI passthrough later on, but that change should be done in that series and not preemptively here. > case GICR_IIDR: > if ( dabt.size != DABT_WORD ) goto bad_width; > @@ -106,11 +118,16 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 2) << 48 | > MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 1) << 40 | > MPIDR_AFFINITY_LEVEL(v->arch.vmpidr, 0) << 32); > + if ( gic_lpi_supported() ) > + { > + /* Set LPI support */ > + aff |= GICR_TYPER_PLPIS; > + /* GITS_TYPER.PTA is 0. Provice vcpu number as ta */ "Provide" and an extra space before "0". > + aff |= (v->vcpu_id << GICR_TYPER_PROCESSOR_SHIFT); > + } > *r = aff; > - > if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST ) > *r |= GICR_TYPER_LAST; > - Spurious whitespace changes. > return 1; > case GICR_STATUSR: > /* Not implemented */ > @@ -125,10 +142,21 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info, > /* WO. Read as zero */ > goto read_as_zero_64; > case GICR_PROPBASER: > - /* LPI's not implemented */ > + if ( gic_lpi_supported() ) > + { > + if ( dabt.size != DABT_DOUBLE_WORD ) goto bad_width; > + /* Remove shareability attribute we don't want dom to flush */ This code doesn't appear to match the code, nothing is removing an attribute here. Perhaps it belongs next to the place which initialises, or handles writes to, v->domain->arch.vits->propbase? > @@ -203,7 +231,18 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info, > switch ( gicr_reg ) > { > case GICR_CTLR: > - /* LPI's not implemented */ > + if ( gic_lpi_supported() ) > + { > + /* > + * Enable LPI's for ITS. Direct injection of LPI > + * by writing to GICR_{SET,CLR}LPIR are not supported "is not supported" > + */ > + if ( dabt.size != DABT_WORD ) goto bad_width; > + vgic_lock(v); > + v->domain->arch.vgic.gicr_ctlr = (*r) & GICR_CTLR_ENABLE_LPIS; Extra space after the &. > @@ -694,6 +755,14 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, mmio_info_t *info) > *r = ((ncpus - 1) << GICD_TYPE_CPUS_SHIFT | > DIV_ROUND_UP(v->domain->arch.vgic.nr_spis, 32)); > > + if ( gic_lpi_supported() ) > + { > + irq_bits = gic_nr_id_bits(); > + *r |= GICD_TYPE_LPIS; > + } > + else > + irq_bits = get_count_order(vgic_num_irqs(v->domain)); I think gic_nr_id_bits should return the correct thing whether or not LPIs are supported, i.e. if ( gic_lpi_supported() ) *r |= GICD_TYPE_LPIS; irq_bits = gic_nr_id_bits(); should be sufficient. > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 67e4695..49db7f0 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -102,6 +102,7 @@ struct arch_domain > paddr_t dbase; /* Distributor base address */ > paddr_t cbase; /* CPU base address */ > #ifdef CONFIG_ARM_64 > + int gicr_ctlr; Wrong indentation Ian.