From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v7a 13/14] xen/arm: split vgic driver into generic and vgic-v2 driver Date: Wed, 02 Jul 2014 10:01:52 +0100 Message-ID: <53B3CA80.6070901@linaro.org> References: <1404196882-23473-1-git-send-email-vijay.kilari@gmail.com> <1404196882-23473-14-git-send-email-vijay.kilari@gmail.com> <53B2B2D9.8000906@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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 Cc: Ian Campbell , Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , Tim Deegan , "xen-devel@lists.xen.org" , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On 02/07/14 07:39, Vijay Kilari wrote: > On Tue, Jul 1, 2014 at 6:38 PM, Julien Grall wrote: > >>> -static int vgic_to_sgi(struct vcpu *v, register_t sgir) >>> +/* TODO: unsigned long is used to fit vcpu_mask. Change to cpu_mask */ >>> +int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, >>> + unsigned long vcpu_mask) >> >> >> [..] >> >>> + case SGI_TARGET_OTHERS: >> >> [..] >> >>> + break; >>> + case SGI_TARGET_SELF: >>> + set_bit(current->vcpu_id, &vcpu_mask); >> >> I already said it on V4, V5 and now V6a and don't see any change or >> comment from you side. >> >> For SGI_TARGET_{OTHERS,SELF}, you can't assume that vcpu_mask will be >> equal to 0... >> >> It comes directly guest write to GICD_SIGR. Please make sure to handle >> it correctly or the guest could send SGI to the wrong VCPUs. > > Sorry I missed this patch to fix the comments. > > First, if Guest uses this series of patches, mask is not written to GICD_SGIR > for OTHERS & SELF case in v2 driver That's not a reason to make buggy assumption. You will have to fix some day for another GIC driver. It's better to fix it today otherwise we will forget it later. > static void gicv2_send_SGI(enum gic_sgi sgi, enum gic_sgi_mode irqmode, > const cpumask_t *cpu_mask) > { > unsigned int mask = 0; > > switch ( irqmode ) > { > case SGI_TARGET_OTHERS: > writel_relaxed(GICD_SGI_TARGET_OTHERS | sgi, GICD + GICD_SGIR); > break; > case SGI_TARGET_SELF: > writel_relaxed(GICD_SGI_TARGET_SELF | sgi, GICD + GICD_SGIR); > break; > > Even if Guest writes some values to mask, then only way I check is to forcefully > make vcpu_mask as 0 for OTHERS & SELF case before using it with warning For the GICv2 POV it's perfectly valid to write garbage in CPUTargetList while we send an SGI to ourself. I'm fine with vcpu_mask = 0, but the warning is not useful as it's not a bug. >> >> [..] >> >>> +struct vgic_ops { >>> + /* Initialize vGIC */ >>> + int (*vcpu_init)(struct vcpu *v); >>> + /* Domain specific initialization of vGIC */ >>> + int (*domain_init)(struct domain *d); >>> + /* SGI handler of vGIC */ >>> + int (*send_sgi)(struct vcpu *v, register_t sgir); >> >> You've introduced vgic_send_sgi here, I'm not sure to understand why... >> Can you give more input? > > You have asked me make sending sgi as a callback. > So I have introduced the following way > > On GICD_SGIR write by guest, vgic-v2.c calls vgic_send_sgi() > which will call the registered callback (*send_sgi). > Here send_sgi is registered with vgic_v2_to_sgi() which will inject > sgi IIRC, I've asked this change for the SGI support on VGICv3 to handle sysreg per vGIC. IHMO, here it's as no-sense because the usage looks useless. I would like to see your GICv3 series before saying this is OK. >> >>> +}; >>> + >>> /* Number of ranks of interrupt registers for a domain */ >>> #define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32) >>> >>> @@ -133,6 +142,8 @@ static inline void vgic_byte_write(uint32_t *reg, uint32_t var, int offset) >>> *reg |= var; >>> } >>> >>> +extern enum gic_sgi_mode irqmode; >>> + >> >> Hu? What is used for? > > This is forward declaration for below function in vgic.h file. May > be extern is not required here > extern int vgic_to_sgi(struct vcpu *v, register_t sgir, > enum gic_sgi_mode irqmode, int virq, > unsigned long vcpu_mask); Forward declaration of what? Here, you are defining an external variable "irqmode" which is never used. Regards, -- Julien Grall