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.
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()).
>
> @@ -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?
Thanks.
Peter
