Re: [Qemu-devel] [PATCH v3 3/8] intel_iommu: pass whole remapped addresses to apic

2016-10-09 Thread Peter Xu 
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ář  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ář 
> > > > ---
> > > > 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(_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();
> > > >  }
> > > >  
> > > >  /* 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 & 0xff00;
> > > what about BE host? should it be:
> > > 
> > >  msg.__addr_hi = cpu_to_le32(irq->dest & 0xff00)
> > > 
> > > 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
> >

Re: [Qemu-devel] [PATCH v3 3/8] intel_iommu: pass whole remapped addresses to apic

2016-10-09 Thread Michael S. Tsirkin 
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ář  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ář 
> > > ---
> > > 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(_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();
> > >  }
> > >  
> > >  /* 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 & 0xff00;
> > what about BE host? should it be:
> > 
> >  msg.__addr_hi = cpu_to_le32(irq->dest & 0xff00)
> > 
> > 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.

-- 
MST

Re: [Qemu-devel] [PATCH v3 3/8] intel_iommu: pass whole remapped addresses to apic

2016-10-07 Thread Peter Xu 
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ář  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ář 
> > ---
> > 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(_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();
> >  }
> >  
> >  /* 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 & 0xff00;
> what about BE host? should it be:
> 
>  msg.__addr_hi = cpu_to_le32(irq->dest & 0xff00)
> 
> 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

Re: [Qemu-devel] [PATCH v3 3/8] intel_iommu: pass whole remapped addresses to apic

2016-10-04 Thread Igor Mammedov 
On Fri, 30 Sep 2016 18:10:08 +0200
Radim Krčmář  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ář 
> ---
> 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(_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();
>  }
>  
>  /* 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 & 0xff00;
what about BE host? should it be:

 msg.__addr_hi = cpu_to_le32(irq->dest & 0xff00)

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(_space_memory, to.address,
> - , size)) {
> -VTD_DPRINTF(GENERAL, "error: fail to write 0x%"PRIx64
> -" value 0x%"PRIx32, to.address, to.data);
> -}
> +apic_get_class()->send_msi();
>  
>  return MEMTX_OK;
>  }

[Qemu-devel] [PATCH v3 3/8] intel_iommu: pass whole remapped addresses to apic

2016-09-30 Thread Radim Krčmář 
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ář 
---
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(_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();
 }
 
 /* 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 & 0xff00;
 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(_space_memory, to.address,
- , size)) {
-VTD_DPRINTF(GENERAL, "error: fail to write 0x%"PRIx64
-" value 0x%"PRIx32, to.address, to.data);
-}
+apic_get_class()->send_msi();
 
 return MEMTX_OK;
 }
-- 
2.10.0