From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v4 12/16] xen/arm: split vgic driver into generic and vgic-v2 driver Date: Tue, 27 May 2014 17:50:34 +0100 Message-ID: <5384C25A.4060709@linaro.org> References: <1401100009-7326-1-git-send-email-vijay.kilari@gmail.com> <1401100009-7326-13-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: <1401100009-7326-13-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, xen-devel@lists.xen.org Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com List-Id: xen-devel@lists.xenproject.org Hi Vijay, On 05/26/2014 11:26 AM, vijay.kilari@gmail.com wrote: > +static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info) > +{ I didn't review closer this function. I guess it's a simple copy from vgic_distr_mmio_read? [..] > + /* Reserved -- read as zero */ > + case 0x00c ... 0x01c: > + case 0x040 ... 0x07c: > + case 0x7fc: > + case 0xbfc: > + case 0xf04 ... 0xf0c: > + case 0xf30 ... 0xfcc: > + goto read_as_zero; Hrrmm ... actually you dropped REG( ). Please specify every big changes you made in the commit message. [..] > +static int vgic_v2_to_sgi(struct vcpu *v, register_t sgir) > +{ > + > + int virq; > + int irqmode; > + unsigned long vcpu_mask = 0; > + > + irqmode = (sgir >> GICD_SGI_TARGET_LIST_SHIFT) & GICD_SGI_TARGET_LIST_MASK; irqmode is an enum gic_sgi_mode, right? If so the type should be irqmode. Futhermore in vgic_to_sgi you are using SGI_TARGET_* which as no assigned value (see your enum). It doesn't sound right to blindly store the architectural value in this enum... > + virq = (sgir & GICD_SGI_INTID_MASK); > + vcpu_mask = (sgir & GICD_SGI_TARGET_MASK) >> GICD_SGI_TARGET_SHIFT; > + > + return vgic_to_sgi(v, sgir, irqmode, virq, vcpu_mask); > +} > + > +static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info) > +{ Same remark vgic_v2_distr_mmio_write. I didn't review closer this function. [..] > + case GICD_ICFGR: /* SGIs */ > + goto write_ignore; > + case GICD_ICFGR + 1: /* PPIs */ > + /* It is implementation defined if these are writeable. We chose not */ > + goto write_ignore; > + case GICD_ICFGR + 2 ... GICD_ICFGRN: /* SPIs */ > + if ( dabt.size != DABT_WORD ) goto bad_width; > + rank = vgic_irq_rank(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD); > + vgic_lock_rank(v, rank); > + if ( rank == NULL) goto write_ignore; You've reintroduce the XSA-94 here (see bf70db7 vgic: Check rank in GICD_ICFGR* emulation before locking). When you send a new version of a serie, please check there is no update on this code which may fix error. I saw you shared a part of the emulation between the distributor and the redistributor in GICv3. I think you can also share with GICv2, this could avoid fix in 2 places the same bug (or worst only fixing in 1 place). [..] > -static int vgic_to_sgi(struct vcpu *v, register_t sgir) > +int vgic_to_sgi(struct vcpu *v, register_t sgir, int irqmode, int virq, irqmode should be enum gic_sgi_mode. > + unsigned long vcpu_mask) You can't assume that all the VCPU bits will fit in an unsigned long. We will have to use cpumask_t at some point. I'm fine if you don't handle it for now, but you need to write down somewhere the limitation of this function. [..] > + case SGI_TARGET_OTHERS: > + case SGI_TARGET_SELF: For this 2 case, you can't assume that vcpu_mask will be equal to 0... It comes from the GICD_SGIR... > + default: > + gdprintk(XENLOG_WARNING, "vGICD: unhandled GICD_SGIR write %"PRIregister" with wrong mode\n", > + sgir); > + return 0; > } > > for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus ) > { > if ( !is_vcpu_running(d, vcpuid) ) > { > - gdprintk(XENLOG_WARNING, "vGICD: GICD_SGIR write r=%"PRIregister" vcpu_mask=%lx, wrong CPUTargetList\n", > + gdprintk(XENLOG_WARNING, " write r=%"PRIregister" vcpu_mask=%lx, wrong CPUTargetList\n", For clarity, I would keep at least vGIC in the warning message. [..] > @@ -654,7 +254,6 @@ out: > int domain_vgic_init(struct domain *d) > { > int i; > - Spurious change? And also invalid following the coding style. > d->arch.vgic.ctlr = 0; > > /* Currently nr_lines in vgic and gic doesn't have the same meanings > @@ -665,20 +264,34 @@ int domain_vgic_init(struct domain *d) > else > d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */ > > + if ( gic_hw_version() == GIC_V2 ) > + vgic_v2_init(d); > + else > + panic("No VGIC found\n"); As said on V3, panic is not the right solution in a domain creation code path. You should return here. Futhermore, gic_hw_version is returning an enum. I would use switch/case there. [..] > + if ( d->arch.vgic.pending_irqs == NULL ) > + { > + xfree(d->arch.vgic.shared_irqs); > + return -ENOMEM; > + } > + Hrrrmmm... this change has been pused more than one month ago on master. Regards, -- Julien Grall