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

[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ář <address@hidden> 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ář <address@hidden>
> > > > ---
> > > > 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