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