[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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)




+        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 )
         writel_relaxed(GICD_SGI_TARGET_OTHERS | sgi, GICD + GICD_SGIR);
     case SGI_TARGET_SELF:
         writel_relaxed(GICD_SGI_TARGET_SELF | sgi, GICD + GICD_SGIR);

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

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.


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.