Re: [Qemu-devel] [PATCH 1/2] i386: Prepare for interrupt remapping

[Jan Kiszka]

Hi Peter,

On 2016-03-10 06:18, Peter Xu wrote:
> Hi, Jan/Rita,
> 
> Have not gone deeper... Got several comments and questions inline.
> 
> On Wed, Mar 09, 2016 at 12:58:41AM +0530, Rita Sinha wrote:
> 
> [...]
> 
>> +static AddressSpace *get_dma_address_space(void)
>> +{
>> +    return &PC_MACHINE(qdev_get_machine())->dma_address_space;
>> +}
>> +
>>  /* Given the reg addr of both the message data and address, generate an
>>   * interrupt via MSI.
>>   */
>> @@ -282,7 +288,7 @@ static void vtd_generate_interrupt(IntelIOMMUState *s, 
>> hwaddr mesg_addr_reg,
>>      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,
>> +    address_space_stl_le(get_dma_address_space(), addr, data,
>>                           MEMTXATTRS_UNSPECIFIED, NULL);
>>  }
> 
> Would this work? AFAIU, IOMMU generated fault interrupts does not
> need any translation at all.

get_dma_address_space() returns the native one, untranslated. If you
look at the succeeding patch, we replace the address spaces of those
devices that are under IOMMU control. And the IOMMU continues to use
this one.

> 
> One more question about the design itself: I see that one new AS is
> created for DMA address space named dma_address_space. Could you
> help explain why we need this? I am still naive on QEMU memory, what
> I feel is that, current memory framework can work nicely without
> this extra address space, using existing address translation
> mechanisms, like the implementation in the following patch:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04393.html
> 
> With the new address space, we will need more loops when doing
> memory address translation for IR (in address_space_translate()). 

At the time of designing this (about 1.5 years ago), there were no
memory region attributes yet. So the per device address spaces also
helped with identifying MSI request sources. Of course, they also helped
with modelling which devices get remapped and which not. We need to
rethink this now, in the light of memory region attributes.

> 
>>  
>> @@ -496,7 +502,7 @@ static int vtd_get_root_entry(IntelIOMMUState *s, 
>> uint8_t index,
>>      dma_addr_t addr;
>>  
>>      addr = s->root + index * sizeof(*re);
>> -    if (dma_memory_read(&address_space_memory, addr, re, sizeof(*re))) {
>> +    if (dma_memory_read(get_dma_address_space(), addr, re, sizeof(*re))) {
> 
> For memory reads from IOMMU, I suppose we do not need translation as
> well? I think this should work though, IMHO is because you did not
> implement read() op for int_remap_as.  So, this read will fall
> through to system address space finally, just like what happened
> before this change.
> 
>>          VTD_DPRINTF(GENERAL, "error: fail to access root-entry at 0x%"PRIx64
>>                      " + %"PRIu8, s->root, index);
>>          re->val = 0;
>> @@ -521,7 +527,7 @@ static int vtd_get_context_entry_from_root(VTDRootEntry 
>> *root, uint8_t index,
>>          return -VTD_FR_ROOT_ENTRY_P;
>>      }
>>      addr = (root->val & VTD_ROOT_ENTRY_CTP) + index * sizeof(*ce);
>> -    if (dma_memory_read(&address_space_memory, addr, ce, sizeof(*ce))) {
>> +    if (dma_memory_read(get_dma_address_space(), addr, ce, sizeof(*ce))) {
> 
> Same as above. Will skip all similiar ones.
> 
> [...]
> 
>>  static void kvm_apic_reset(APICCommonState *s)
>> @@ -182,8 +186,10 @@ static void kvm_apic_realize(DeviceState *dev, Error 
>> **errp)
>>  {
>>      APICCommonState *s = APIC_COMMON(dev);
>>  
>> -    memory_region_init_io(&s->io_memory, NULL, &kvm_apic_io_ops, s, 
>> "kvm-apic-msi",
>> -                          APIC_SPACE_SIZE);
>> +    memory_region_init(&s->io_memory, NULL, "kvm-apic", APIC_SPACE_SIZE);
>> +
>> +    memory_region_init_io(&s->msi_region, NULL, &kvm_msi_region_ops, NULL,
>> +                          "kvm-msi", MSI_REGION_SIZE);
> 
> I do not quite understand why we need to have two MRs. Could you
> help explain too?

MSI requests from the devices have nothing to do with APIC access from
the CPUs - two different sources, two different target (a CPU can't
trigger MSIs, and devices can't access the APICs). This is currently
mangled due to past limitations of QEMU, and that should be cleaned up
eventually. E.g. by introducing a DMA address spaces.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux