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

[Peter Xu <peterx@redhat.com>]

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