| Message ID | 1462568028-31037-3-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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á? <rkrcmar@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); [...] 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?
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á? <rkrcmar@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. Paolo
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á? <rkrcmar@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); } }
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á? <rkrcmar@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); > } > } > Sounds good. Paolo
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); + + apic_class->deliver_msi(msi); +} + /* Given the reg addr of both the message data and address, generate an * interrupt via MSI. */ static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg, hwaddr mesg_data_reg) { - hwaddr addr; - uint32_t data; + MSIMessage msi; assert(mesg_data_reg < DMAR_REG_SIZE); assert(mesg_addr_reg < DMAR_REG_SIZE); - addr = vtd_get_long_raw(s, mesg_addr_reg); - data = vtd_get_long_raw(s, mesg_data_reg); + msi.address = vtd_get_quad_raw(s, mesg_addr_reg); + msi.data = vtd_get_long_raw(s, mesg_data_reg); VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data); - address_space_stl_le(&address_space_memory, addr, data, - MEMTXATTRS_UNSPECIFIED, NULL); + + apic_deliver_msi(&msi); } /* Generate a fault event to software via MSI if conditions are met. @@ -2113,6 +2123,7 @@ static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out) msg.dest_mode = irq->dest_mode; msg.redir_hint = irq->redir_hint; msg.dest = irq->dest; + msg.__addr_hi = irq->dest & 0xffffff00; msg.__addr_head = 0xfee; /* Keep this from original MSI address bits */ msg.__not_used = irq->msi_addr_last_bits; @@ -2262,11 +2273,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr, VTD_DPRINTF(IR, "delivering MSI 0x%"PRIx64":0x%"PRIx32, to.address, to.data); - if (dma_memory_write(&address_space_memory, to.address, - &to.data, size)) { - VTD_DPRINTF(GENERAL, "error: fail to write 0x%"PRIx64 - " value 0x%"PRIx32, to.address, to.data); - } + apic_deliver_msi(&to); return MEMTX_OK; }
The memory-mapped interface cannot express x2APIC destinations that are a result of remapping. Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- hw/i386/intel_iommu.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)