| Message ID | 20160930161013.9832-4-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 30 Sep 2016 18:10:08 +0200 Radim Krčmář <rkrcmar@redhat.com> wrote: > The MMIO interface to APIC only allowed 8 bit addresses, which is not > enough for 32 bit addresses from EIM remapping. > Intel stored upper 24 bits in the high MSI address, so use the same > technique. The technique is also used in KVM MSI interface. > Other APICs are unlikely to handle those upper bits. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter] > --- > hw/i386/intel_iommu.c | 21 +++++++++------------ > 1 file changed, 9 insertions(+), 12 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 9f4e64af1ad5..c39b62b898d8 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -31,6 +31,7 @@ > #include "hw/i386/x86-iommu.h" > #include "hw/pci-host/q35.h" > #include "sysemu/kvm.h" > +#include "hw/i386/apic_internal.h" > > /*#define DEBUG_INTEL_IOMMU*/ > #ifdef DEBUG_INTEL_IOMMU > @@ -279,18 +280,17 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id, > 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_long_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); > + VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, > + msi.address, msi.data); > + apic_get_class()->send_msi(&msi); > } > > /* Generate a fault event to software via MSI if conditions are met. > @@ -2133,6 +2133,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; what about BE host? should it be: msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00) Also question to Peter, why __addr_hi is not HOST_WORDS_BIGENDIAN conditioned? now we have: struct VTD_MSIMessage { union { struct { #ifdef HOST_WORDS_BIGENDIAN uint32_t __addr_head:12; /* 0xfee */ [...] #else [...] uint32_t __addr_head:12; /* 0xfee */ #endif uint32_t __addr_hi:32; > msg.__addr_head = cpu_to_le32(0xfee); > /* Keep this from original MSI address bits */ > msg.__not_used = irq->msi_addr_last_bits; > @@ -2281,11 +2282,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr, > " for device sid 0x%04x", > to.address, to.data, sid); > > - 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_get_class()->send_msi(&to); > > return MEMTX_OK; > }
On Tue, Oct 04, 2016 at 01:17:28PM +0200, Igor Mammedov wrote: > On Fri, 30 Sep 2016 18:10:08 +0200 > Radim Krčmář <rkrcmar@redhat.com> wrote: > > > The MMIO interface to APIC only allowed 8 bit addresses, which is not > > enough for 32 bit addresses from EIM remapping. > > Intel stored upper 24 bits in the high MSI address, so use the same > > technique. The technique is also used in KVM MSI interface. > > Other APICs are unlikely to handle those upper bits. > > > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > --- > > v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter] > > --- > > hw/i386/intel_iommu.c | 21 +++++++++------------ > > 1 file changed, 9 insertions(+), 12 deletions(-) > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 9f4e64af1ad5..c39b62b898d8 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -31,6 +31,7 @@ > > #include "hw/i386/x86-iommu.h" > > #include "hw/pci-host/q35.h" > > #include "sysemu/kvm.h" > > +#include "hw/i386/apic_internal.h" > > > > /*#define DEBUG_INTEL_IOMMU*/ > > #ifdef DEBUG_INTEL_IOMMU > > @@ -279,18 +280,17 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id, > > 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_long_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); > > + VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, > > + msi.address, msi.data); > > + apic_get_class()->send_msi(&msi); > > } > > > > /* Generate a fault event to software via MSI if conditions are met. > > @@ -2133,6 +2133,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; > what about BE host? should it be: > > msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00) > > Also question to Peter, why __addr_hi is not HOST_WORDS_BIGENDIAN conditioned? > now we have: > struct VTD_MSIMessage { > union { > struct { > #ifdef HOST_WORDS_BIGENDIAN > uint32_t __addr_head:12; /* 0xfee */ > [...] > #else > [...] > uint32_t __addr_head:12; /* 0xfee */ > #endif > uint32_t __addr_hi:32; I think __addr_hi is not a bit field at all. It'll be the same if I put it into the block. E.g., it'll look like: #ifdef HOST_WORDS_BIGENDIAN uint32_t __addr_head:12; /* 0xfee */ uint32_t dest:8; uint32_t __reserved:8; uint32_t redir_hint:1; uint32_t dest_mode:1; uint32_t __not_used:2; uint32_t __addr_hi:32; #else uint32_t __not_used:2; uint32_t dest_mode:1; uint32_t redir_hint:1; uint32_t __reserved:8; uint32_t dest:8; uint32_t __addr_head:12; /* 0xfee */ uint32_t __addr_hi:32; #endif Only real bit fields (like dest_mode, redir_hint, etc.) order is handled differently on BE/LE machines. Since __addr_hi is not a bit field (it's typed as u32, and it's 32 bits long), it should always be using a higher address comparing to above real bit fields, so no ordering issue AFAIK. I have patch in my queue that fixes this (change "__addr_hi:32" to "__addr_hi"). Though it should not be a big problem. -- peterx
On Sat, Oct 08, 2016 at 01:24:55PM +0800, Peter Xu wrote: > On Tue, Oct 04, 2016 at 01:17:28PM +0200, Igor Mammedov wrote: > > On Fri, 30 Sep 2016 18:10:08 +0200 > > Radim Krčmář <rkrcmar@redhat.com> wrote: > > > > > The MMIO interface to APIC only allowed 8 bit addresses, which is not > > > enough for 32 bit addresses from EIM remapping. > > > Intel stored upper 24 bits in the high MSI address, so use the same > > > technique. The technique is also used in KVM MSI interface. > > > Other APICs are unlikely to handle those upper bits. > > > > > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > > --- > > > v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter] > > > --- > > > hw/i386/intel_iommu.c | 21 +++++++++------------ > > > 1 file changed, 9 insertions(+), 12 deletions(-) > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > index 9f4e64af1ad5..c39b62b898d8 100644 > > > --- a/hw/i386/intel_iommu.c > > > +++ b/hw/i386/intel_iommu.c > > > @@ -31,6 +31,7 @@ > > > #include "hw/i386/x86-iommu.h" > > > #include "hw/pci-host/q35.h" > > > #include "sysemu/kvm.h" > > > +#include "hw/i386/apic_internal.h" > > > > > > /*#define DEBUG_INTEL_IOMMU*/ > > > #ifdef DEBUG_INTEL_IOMMU > > > @@ -279,18 +280,17 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id, > > > 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_long_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); > > > + VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, > > > + msi.address, msi.data); > > > + apic_get_class()->send_msi(&msi); > > > } > > > > > > /* Generate a fault event to software via MSI if conditions are met. > > > @@ -2133,6 +2133,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; > > what about BE host? should it be: > > > > msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00) > > > > Also question to Peter, why __addr_hi is not HOST_WORDS_BIGENDIAN conditioned? > > now we have: > > struct VTD_MSIMessage { > > union { > > struct { > > #ifdef HOST_WORDS_BIGENDIAN > > uint32_t __addr_head:12; /* 0xfee */ > > [...] > > #else > > [...] > > uint32_t __addr_head:12; /* 0xfee */ > > #endif > > uint32_t __addr_hi:32; > > I think __addr_hi is not a bit field at all. It'll be the same if I > put it into the block. E.g., it'll look like: > > #ifdef HOST_WORDS_BIGENDIAN > uint32_t __addr_head:12; /* 0xfee */ > uint32_t dest:8; > uint32_t __reserved:8; > uint32_t redir_hint:1; > uint32_t dest_mode:1; > uint32_t __not_used:2; > uint32_t __addr_hi:32; > #else > uint32_t __not_used:2; > uint32_t dest_mode:1; > uint32_t redir_hint:1; > uint32_t __reserved:8; > uint32_t dest:8; > uint32_t __addr_head:12; /* 0xfee */ > uint32_t __addr_hi:32; > #endif > > Only real bit fields (like dest_mode, redir_hint, etc.) order is > handled differently on BE/LE machines. Since __addr_hi is not a bit > field (it's typed as u32, and it's 32 bits long), it should always be > using a higher address comparing to above real bit fields, so no > ordering issue AFAIK. > > I have patch in my queue that fixes this (change "__addr_hi:32" to > "__addr_hi"). Though it should not be a big problem. > > -- peterx IMHO it's best to avoid bitfields completely. Use two uint32_t fields and add functions to pack/unpack sub-fields.
On Sun, Oct 09, 2016 at 11:47:57PM +0300, Michael S. Tsirkin wrote: > On Sat, Oct 08, 2016 at 01:24:55PM +0800, Peter Xu wrote: > > On Tue, Oct 04, 2016 at 01:17:28PM +0200, Igor Mammedov wrote: > > > On Fri, 30 Sep 2016 18:10:08 +0200 > > > Radim Krčmář <rkrcmar@redhat.com> wrote: > > > > > > > The MMIO interface to APIC only allowed 8 bit addresses, which is not > > > > enough for 32 bit addresses from EIM remapping. > > > > Intel stored upper 24 bits in the high MSI address, so use the same > > > > technique. The technique is also used in KVM MSI interface. > > > > Other APICs are unlikely to handle those upper bits. > > > > > > > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > > > --- > > > > v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter] > > > > --- > > > > hw/i386/intel_iommu.c | 21 +++++++++------------ > > > > 1 file changed, 9 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > > > index 9f4e64af1ad5..c39b62b898d8 100644 > > > > --- a/hw/i386/intel_iommu.c > > > > +++ b/hw/i386/intel_iommu.c > > > > @@ -31,6 +31,7 @@ > > > > #include "hw/i386/x86-iommu.h" > > > > #include "hw/pci-host/q35.h" > > > > #include "sysemu/kvm.h" > > > > +#include "hw/i386/apic_internal.h" > > > > > > > > /*#define DEBUG_INTEL_IOMMU*/ > > > > #ifdef DEBUG_INTEL_IOMMU > > > > @@ -279,18 +280,17 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id, > > > > 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_long_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); > > > > + VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, > > > > + msi.address, msi.data); > > > > + apic_get_class()->send_msi(&msi); > > > > } > > > > > > > > /* Generate a fault event to software via MSI if conditions are met. > > > > @@ -2133,6 +2133,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; > > > what about BE host? should it be: > > > > > > msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00) > > > > > > Also question to Peter, why __addr_hi is not HOST_WORDS_BIGENDIAN conditioned? > > > now we have: > > > struct VTD_MSIMessage { > > > union { > > > struct { > > > #ifdef HOST_WORDS_BIGENDIAN > > > uint32_t __addr_head:12; /* 0xfee */ > > > [...] > > > #else > > > [...] > > > uint32_t __addr_head:12; /* 0xfee */ > > > #endif > > > uint32_t __addr_hi:32; > > > > I think __addr_hi is not a bit field at all. It'll be the same if I > > put it into the block. E.g., it'll look like: > > > > #ifdef HOST_WORDS_BIGENDIAN > > uint32_t __addr_head:12; /* 0xfee */ > > uint32_t dest:8; > > uint32_t __reserved:8; > > uint32_t redir_hint:1; > > uint32_t dest_mode:1; > > uint32_t __not_used:2; > > uint32_t __addr_hi:32; > > #else > > uint32_t __not_used:2; > > uint32_t dest_mode:1; > > uint32_t redir_hint:1; > > uint32_t __reserved:8; > > uint32_t dest:8; > > uint32_t __addr_head:12; /* 0xfee */ > > uint32_t __addr_hi:32; > > #endif > > > > Only real bit fields (like dest_mode, redir_hint, etc.) order is > > handled differently on BE/LE machines. Since __addr_hi is not a bit > > field (it's typed as u32, and it's 32 bits long), it should always be > > using a higher address comparing to above real bit fields, so no > > ordering issue AFAIK. > > > > I have patch in my queue that fixes this (change "__addr_hi:32" to > > "__addr_hi"). Though it should not be a big problem. > > > > -- peterx > > IMHO it's best to avoid bitfields completely. Use two uint32_t fields > and add functions to pack/unpack sub-fields. Yeah I now found that bitfield is error-prone. Will take your advice in the coming patches when capable. Thanks, -- peterx
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 9f4e64af1ad5..c39b62b898d8 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -31,6 +31,7 @@ #include "hw/i386/x86-iommu.h" #include "hw/pci-host/q35.h" #include "sysemu/kvm.h" +#include "hw/i386/apic_internal.h" /*#define DEBUG_INTEL_IOMMU*/ #ifdef DEBUG_INTEL_IOMMU @@ -279,18 +280,17 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id, 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_long_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); + VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, + msi.address, msi.data); + apic_get_class()->send_msi(&msi); } /* Generate a fault event to software via MSI if conditions are met. @@ -2133,6 +2133,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 = cpu_to_le32(0xfee); /* Keep this from original MSI address bits */ msg.__not_used = irq->msi_addr_last_bits; @@ -2281,11 +2282,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr, " for device sid 0x%04x", to.address, to.data, sid); - 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_get_class()->send_msi(&to); return MEMTX_OK; }
The MMIO interface to APIC only allowed 8 bit addresses, which is not enough for 32 bit addresses from EIM remapping. Intel stored upper 24 bits in the high MSI address, so use the same technique. The technique is also used in KVM MSI interface. Other APICs are unlikely to handle those upper bits. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter] --- hw/i386/intel_iommu.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)