Re: [Qemu-devel] [PATCH 2/4] intel_iommu: use deliver_msi APIC callback

[Paolo Bonzini]

On 17/05/2016 15:53, Eduardo Habkost wrote:
> 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ář <address@hidden>
>>>> ---
>>>>  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);
>         }
>     }
> 

Sounds good.

Paolo