Re: [Xen-devel] [PATCH v7a 13/14] xen/arm: split vgic driver into generic and vgic-v2 driver

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 02/07/14 07:39, Vijay Kilari wrote:
> On Tue, Jul 1, 2014 at 6:38 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel