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

[Igor Mammedov]

On Sat, 8 Oct 2016 14:14:09 +0800
Peter Xu <address@hidden> wrote:

> On Fri, Oct 07, 2016 at 06:24:15PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
> > KVM accepts the address in host endianess and QEMU/VTD code also uses
> > host endianess for internal representation of memory addresses, so this
> > hunk should be fine.
> > 
> > It is confusing, because the VTD is definitely broken with respect to
> > endianess -- it is even trying to swap the order of bits in a byte in
> > the definition of VTD_MSIMessage.
> > I don't believe that dma_memory_write() accepted LE address on BE hosts,
> > so the existing code for filling the address is wrong:
> > 
> >       msg.__addr_head = cpu_to_le32(0xfee);  
> 
> Yeah. This is my fault. Sorry for the troubles.
> 
> I have a patch (as well...) to fix this in my local tree, but not
> posted (as mst suggested). Maybe it's time to post some of them now (I
> tried to make patches more into a bunch so that they won't be lost in
> mailing list in case maintainer missed it). I agree that current code
> is never tested on big endian machines yet.
> 
> Here it should be:
> 
>     msg.__addr_head = 0xfee;
> 
> >   
> > >                     should it be:
> > >  msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00)  
> > 
> > I don't think so.
> > 
> > Howewer, there are endianess bugs in this patch:
> >   
> > >> @@ -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);  
> > 
> > because dma_memory_write() does magic on data.
> > 
> > I don't understand how the code should have worked before this series,
> > because kvm_apic_mem_write() expects data in little endian and
> > apic_mem_writel() expects data in host endian, even though both of them
> > are DEVICE_NATIVE_ENDIAN and are called the same way, so one shouldn't
> > work.  
> 
> I guess that's because APIC is only used for x86? Then it does not
> matter. And I agree with you that currently MSI is a little bit
> confused on endianess.
> 
> First of all, I believe in the protocol MSI (along with PCI logics)
> should be LE.
> 
> Instead, our MSIMessage struct looks more like to be for host
> endianess. E.g. in msi_get_message() we are using:
> 
>     msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> 
> and pci_get_word() is converting LE to host endianess.
> 
> However, in all kvm_irqchip_*() APIs, we are assuming MSIMessage as
> LE. E.g.:
> 
> int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> {
>     struct kvm_msi msi;
>     KVMMSIRoute *route;
> 
>     if (kvm_direct_msi_allowed) {
>         ...
>         msi.data = le32_to_cpu(msg.data);
>         ...
>     }
> }
> 
> These things are conflicting.
> 
> Maybe we need to clean this up. And I prefer MSIMessage to be host
> endianess.
Mostly internal QEMU structures are in host order and converted
to guest byte-order on transfer, except for structures that are
memcpy-ed, in which case they are typically marked QEMU_PACKED
and that throws flag to reviewers to check if endianess is correct.

At least struct VTD_MSIMessage definition need a comment saying
what byte-order is expected.

> 
> > 
> > And similarly, this hunk is wrong:
> >   
> > >> @@ -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);
> > >>  }  
> > 
> > It should have been wrong even before, because address_space_stl_le()
> > seems to accept the address in host endianess and not in LE ... UGH.  
> 
> Again, I guess no one is running VT-d in BE machines. So problems are
> not exposed.
> 
> Thanks,
> 
> -- peterx
>