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

[Peter Xu]

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.

> 
> 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