Date: Wed, 11 Dec 2013 12:38:11 -0500 From: John Baldwin <jhb@freebsd.org> To: Neel Natu <neelnatu@gmail.com> Cc: "freebsd-virtualization@freebsd.org" <virtualization@freebsd.org> Subject: Re: [PATCH] Additional lapic support for bhyve Message-ID: <201312111238.12232.jhb@freebsd.org> In-Reply-To: <CAFgRE9ERoqD7GfAaSDe95DKM=3MJhjTBwBz91fFKPpw7c4wp1A@mail.gmail.com> References: <201312101527.47234.jhb@freebsd.org> <CAFgRE9ERoqD7GfAaSDe95DKM=3MJhjTBwBz91fFKPpw7c4wp1A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday, December 11, 2013 2:19:14 am Neel Natu wrote: > Hi John, > > On Tue, Dec 10, 2013 at 12:27 PM, John Baldwin <jhb@freebsd.org> wrote: > > This patch extends the local APIC emulation a bit by adding the following > > things: > > - Add an ioctl and a bhyvectl command to trigger local interrupts on a > > local APIC. The "fixed" and "NMI" delivery modes are enabled. > > - Add support for the CMCI LVT entry. > > - Add the ability to report local APIC errors and trigger errors for > > invalid vectors when sending IPIs or firing an external interrupt that > > references an invalid IDT vector. > > - Silently force all reserved fields in LVT entries to zero when they > > are written (removes the need for clearing those bits when triggering > > an LVT interrupt). > > - Add entries to the MP Table and MADT to advertise the typical x86 LINT > > configuration (ExtINT on LINT0 and NMI on LINT1). > > - Add a bhyvectl command to inject an NMI on an arbitrary CPU (this latter > > should probably be a separate patch) > > > > In particular, while bhyectl --inject-nmi can inject an NMI on a single vcpu, > > the more traditional way of signalling a system-wide error (such as SERR# > > or PERR#) is to assert the LINT1 pins on all CPUs. This can now be done via > > 'bhyectl --vm=foo --cpu=-1 --assert-lapic-lvt=1' (cpu of -1 is a broadcast > > to all CPUs for the LVT ioctl). The patch is at > > people.freebsd.org/~jhb/patches/bhyve_lapic.patch I'm certainly open to > > suggestions on ways to make this be more consistent with the style/design/flow > > of the existing code. > > > > This looks good. > > vlapic_fire_lvt() and vlapic_set_error() can call each other > recursively if guest has programmed the ERROR_LVT vector incorrectly. > This can probably be fixed by passing the LVT index to > vlapic_fire_lvt() and treating LVT_ERROR specially. Oh, cute, and very true. Another option might be to have set_error set a per vlapic flag and avoid posting the interrupt if it detects recursion. > A couple of nits: > - Could you add a /* fallthrough */ comment after setting the 'mask' > for the LINT_LVT entries in vlapic_set_lvt()? Sure. > - Any reason that vlapic_set_error() and vlapic_fire_cmci() are not > static to vlapic.c? Do you anticipate calling them from outside this > file in the future? Yes. I imagine the machine check injection code I plan to work on would be in a different file, and it would need to be able to trigger CMCI. For vlapic_set_error(), there is one error case we currently do not handle which is access to an invalid register (right now we ignore those). For x2APIC we should be generating general protection fault. For mmio access an error is supposed to be asserted instead. I figured for the latter that it would be done in lapic_mmio_read() and lapic_mmio_write(). -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201312111238.12232.jhb>