Re: [Xen-devel] [PATCH RFC v1 33/74] x86/guest: enable event channels upcalls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:05, <wei.liu2@xxxxxxxxxx> wrote:
> @@ -30,6 +31,7 @@
>  bool xen_guest;
>  
>  static uint32_t xen_cpuid_base;
> +static uint8_t evtchn_upcall_vector;

There being a single global vector, why do you use
HVMOP_set_evtchn_upcall_vector instead of setting
HVM_PARAM_CALLBACK_IRQ? Aiui this would also make ...

> @@ -91,9 +93,81 @@ static void map_shared_info(struct e820map *e820)
>      set_fixmap(FIX_XEN_SHARED_INFO, frame);
>  }
>  
> +static void xen_evtchn_upcall(struct cpu_user_regs *regs)
> +{
> +    unsigned int cpu = smp_processor_id();
> +    struct vcpu_info *vcpu_info = &XEN_shared_info->vcpu_info[cpu];
> +
> +    vcpu_info->evtchn_upcall_pending = 0;
> +    xchg(&vcpu_info->evtchn_pending_sel, 0);
> +
> +    ack_APIC_irq();

... this call unnecessary.

Also wouldn't it be better to decouple uses of vcpu_info from
XEN_shared_info right away, for the later extension to more
vCPU-s to be less intrusive?

Also - why xchg() rather than write_atomic() (again further down)?

> +static void ap_setup_event_channels(bool clear)
> +{
> +    unsigned int i, cpu = smp_processor_id();
> +    struct vcpu_info *vcpu_info = &XEN_shared_info->vcpu_info[cpu];
> +    int rc;
> +
> +    ASSERT(evtchn_upcall_vector);
> +    ASSERT(cpu < ARRAY_SIZE(XEN_shared_info->vcpu_info));

Strictly speaking this assertion comes too late. But yes, we have
quite a few such examples elsewhere, so I don't really mind.

> +    if ( !clear )
> +    {
> +        /*
> +         * This is necessary to ensure that a CPU will be interrupted in case
> +         * of an event channel notification.
> +         */
> +        ASSERT(vcpu_info->evtchn_upcall_pending == 0);
> +        ASSERT(vcpu_info->evtchn_pending_sel == 0);
> +    }
> +
> +    rc = xen_hypercall_set_evtchn_upcall_vector(cpu, evtchn_upcall_vector);
> +    if ( rc )
> +        panic("Unable to set evtchn upcall vector: %d", rc);
> +
> +    if ( clear )
> +    {
> +        /*
> +         * Clear any pending upcall bits. This makes us effectively ignore 
> any
> +         * previous upcalls which might be suboptimal.
> +         */
> +        vcpu_info->evtchn_upcall_pending = 0;
> +        xchg(&vcpu_info->evtchn_pending_sel, 0);
> +
> +        /*
> +         * evtchn_pending can be cleared only on the boot CPU because it's
> +         * located in a shared structure.
> +         */
> +        for ( i = 0; i < 8; i++ )

ARRAY_SIZE() (also further down)

I also don't really understand the comment - all CPUs can access
shared info. But then again I don't really understand all this
clearing anyway, including the respective ASSERT()s further up.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel