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

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-10-07 15:05+0200, Igor Mammedov:
> On Wed,  5 Oct 2016 15:06:52 +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.
>> 
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> I don't recall giving my RB to this patch but I do recall asking question,
> see below.

Crap, sorry.

>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> v4: r-b Igor
>> v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter]
>> 
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> @@ -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?

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

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

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.