On Tue, May 17, 2016 at 03:09:49PM +0200, Paolo Bonzini wrote:
>
>
> On 13/05/2016 19:33, Eduardo Habkost wrote:
> > On Fri, May 06, 2016 at 10:53:46PM +0200, Radim Krčmář wrote:
> >> The memory-mapped interface cannot express x2APIC destinations that are
> >> a result of remapping.
> >>
> >> Signed-off-by: Radim Krčmář <rkrc...@redhat.com>
> >> ---
> >> hw/i386/intel_iommu.c | 29 ++++++++++++++++++-----------
> >> 1 file changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index bee85e469477..d10064289551 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -26,6 +26,7 @@
> >> #include "hw/pci/pci.h"
> >> #include "hw/boards.h"
> >> #include "hw/i386/x86-iommu.h"
> >> +#include "hw/i386/apic_internal.h"
> >>
> >> /*#define DEBUG_INTEL_IOMMU*/
> >> #ifdef DEBUG_INTEL_IOMMU
> >> @@ -268,24 +269,33 @@ static void vtd_update_iotlb(IntelIOMMUState *s,
> >> uint16_t source_id,
> >> g_hash_table_replace(s->iotlb, key, entry);
> >> }
> >>
> >> +static void apic_deliver_msi(MSIMessage *msi)
> >> +{
> >> + /* Conjure apic-bound msi delivery out of thin air. */
> >> + X86CPU *cpu = X86_CPU(first_cpu);
> >> + APICCommonState *apic_state = APIC_COMMON(cpu->apic_state);
> >> + APICCommonClass *apic_class = APIC_COMMON_GET_CLASS(apic_state);
> >
> > I see a pattern here:
> >
> > hw/i386/kvmvapic.c-static void do_vapic_enable(void *data)
> > hw/i386/kvmvapic.c-{
> > [...]
> > hw/i386/kvmvapic.c- X86CPU *cpu = X86_CPU(first_cpu);
> > [...]
> > hw/i386/kvmvapic.c: apic_enable_vapic(cpu->apic_state, s->vapic_paddr);
> > [...]
>
> Here first_cpu is just the CPU that do_vapic_enable is being called on
> (via run_on_cpu). So this one can use the posted patch that passes a
> CPUState* to run_on_cpu callbacks.
>
>
> > hw/i386/pc.c-static void pic_irq_request(void *opaque, int irq, int level)
> > hw/i386/pc.c-{
> > hw/i386/pc.c- CPUState *cs = first_cpu;
> > hw/i386/pc.c- X86CPU *cpu = X86_CPU(cs);
> > [...]
> > hw/i386/pc.c: if (cpu->apic_state) {
> > [...]
> >
> >
> > Time to write a common pc_get_first_apic() helper, or provide a
> > PCMachineState::first_apic field?
>
> For this one, we could add a pc_get_apic_class() helper that returns
> NULL if there is no APIC on the first_cpu. It wouldn't be a change for
> this code and would be nicer for Radim's use case.
Returning just the APIC class would be even nicer, yes. We could
just move the type name logic in x86_cpu_apic_create() to
pc_get_apic_class().
About returning NULL if there is no APIC on first_cpu: I would
like to avoid making more code depend on first_cpu if possible.
pc_get_first_apic() wouldn't even need to use
first_cpu/pc_get_apic_class() if we just do this:
CPU_FOREACH(cs) {
cpu = X86_CPU(cs);
if (!cpu->apic_state) {
if (level) {
cpu_interrupt(cs, CPU_INTERRUPT_HARD);
} else {
cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
}
break;
}
if (apic_accept_pic_intr(cpu->apic_state)) {
apic_deliver_pic_intr(cpu->apic_state, level);
}
}
--
Eduardo
