RE: [Xen-devel] [PATCH] make HVM PIC emulation code SMP-safe

["Dong, Eddie" <eddie.dong@xxxxxxxxx>]

Dave:

    Looks like this patch is just using "BUG_ON" for APIs like pic_ioport_read.

    Also let me share something about 1) HVM virtual Interrupt Controller movement in consideration. When APIC is enabled for SMP, guest software need to disable PIC for correct interrupt generation mechanism. If guest bios is presenting MPS, then we can know this through IMCR base on MPS spec when guest switch from PIC mode to Symmetric I/O Mode. If guest bios is presenting ACPI, the guest software will disable PIC by writing 0xff to master PIC IMR. Current VMX guest implements ACPI.

    In either way, when Xen detects the guest has disabled PIC, the PIC IRQ generation logic can be removed for both performance and simplicity. So even VP0 doesn't need to get/queue interrupt from PIC. The ideal solution may be that we need to define some registable APIs in HVM virtual weired interrupt controller logic. At startup time, we regieter PIC APIs, and later on replace it with APIC's (the transition time need special concern while it is non performance critical path).

    Anyway, 2) PIC I/O access may need to be SMP safe (maybe a big lock for simplicity), 3) IOAPIC needs to be SMP safe. Then we try to keep minimal changes to device models with that in Qemu (yes we already have some) for future maintain effort. You are very welcome here.

    thx,eddie

---

From: Dave Lively [mailto:dave.lively@xxxxxxxxx]
Sent: 2006年5月16日 23:06
To: Keir Fraser
Cc: Dong, Eddie; xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: [Xen-devel] [PATCH] make HVM PIC emulation code SMP-safe

Hi Keir / Eddie -

  I was also thinking 
cpu_get_pic_interrupt() should be called only on
VCPU 0 (and in fact, 
originally tested with restriction).  But since I
wasn't sure the 
restriction was necessary (and things worked fine
without it), I removed 
it.  I'll put it back in now, in light of these
comments.  (Revised 
patch attached.)

  However, that doesn't remove the need for locking 
(or some sort
of PIC concurrency control).  First (and most 
importantly), the guest
can access the PIC from *any* VCPU, though it must be 
careful to
access it from at most one VCPU at once.  This alone means it 
can
conflict with hypervisor PIC access from VCPU 0.  (Note that 
the
"APIC kludge" causes PIC acccess from arbitrary VCPUs.  So 
this
patch fixes that as well.  But the patch would be necessary even 
without
this kludge.)

  I'm not wild about having locks on this 
path either.  A safe lockless
protocol may be possible, but I don't have 
the time to try to work one
out now.  In our experience, this patch 
greatly improves the stability 
of SMP guests.  (But note we currently 
tell SMP guest kernels to 
ignore the I/O APIC, otherwise we lose too many 
hda interrupts.)

Dave

P.S. The I/O APIC code has similar issues, 
for mostly the same
reasons (guests can access from any VCPU).  I have a 
similar patch
for it, but haven't been able to test it yet (due to other I/O 
APIC 
problems).

On 5/16/06, Keir Fraser <Keir.Fraser@xxxxxxxxxxxx> wrote:

Well, that would make it behave like a *real* PIC, wired through to
CPU0's INT pin, right? So it sounds good to me, especially if we are
currently spraying PIC interrupts across all VCPUs. And it's even
better if it avoids requiring locking on the vmentry path.

  -- Keir

On 16 May 2006, at 06:20, Dong, Eddie wrote:

> For the PIC I/O access, do weneed lock protect? But for IRQ
> generation, i.e. cpu_get_interrupt, how about following for simplicity
> (not tested)?
>
> diff -r 1e3977e029fd xen/arch/x86/hvm/hvm.c
> --- a/xen/arch/x86/hvm/hvm.c Mon May 08 19:21:41 2006 +0100
> +++ b/xen/arch/x86/hvm/hvm.c Tue May 16 13:18:04 2006 +0800
> @@ -248,7 +248,7 @@ int cpu_get_interrupt(struct vcpu *v, in
> return intno;
> }
> /* read the irq from the PIC */
> - if ( (intno = cpu_get_pic_interrupt(v, type)) != -1 )
> + if ( v->vcpu_id == 0 && (intno = cpu_get_pic_interrupt(v, type))
> != -1 )
> return intno;
>
> return -1;

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel