Re: [Qemu-devel] [PULL v5 29/57] intel_iommu: add SID validation for IR

[Jan Kiszka]

[always keep the list in CC]

On 2016-08-02 10:54, David Kiarie wrote:
> 
> 
> On Tue, Aug 2, 2016 at 11:46 AM, Jan Kiszka <address@hidden
> <mailto:address@hidden>> wrote:
> 
>     On 2016-08-02 10:36, Peter Xu wrote:
>     > On Mon, Aug 01, 2016 at 06:39:05PM +0200, Jan Kiszka wrote:
>     >
>     > [...]
>     >
>     >>>  static MemTxResult vtd_mem_ir_read(void *opaque, hwaddr addr,
>     >>> @@ -2209,11 +2250,17 @@ static MemTxResult vtd_mem_ir_write(void
>     *opaque, hwaddr addr,
>     >>>  {
>     >>>      int ret = 0;
>     >>>      MSIMessage from = {}, to = {};
>     >>> +    uint16_t sid = X86_IOMMU_SID_INVALID;
>     >>>
>     >>>      from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
>     >>>      from.data = (uint32_t) value;
>     >>>
>     >>> -    ret = vtd_interrupt_remap_msi(opaque, &from, &to);
>     >>> +    if (!attrs.unspecified) {
>     >>> +        /* We have explicit Source ID */
>     >>> +        sid = attrs.requester_id;
>     >>> +    }
>     >>
>     >> ...here you fall back to X86_IOMMU_SID_INVALID if writer to this
>     region
>     >> has not provided some valid attrs. That is questionable, defeats
>     >> validation of the IOAPIC e.g. (and you can see lots of
>     >> X86_IOMMU_SID_INVALID in vtd_irte_get when booting a guest).
>     >>
>     >> The credits also go to David who noticed that he still doesn't get a
>     >> proper ID from the IOAPIC while implementing AMD IR. Looks like
>     we need
>     >> to enlighten the IOAPIC MSI writes...
>     >
>     > Jan, David,
>     >
>     > At the time when drafting the patch, I skipped SID verification for
>     > IOAPIC interrupts since it differs from generic PCI devices (no
>     > natural requester ID, so need some hacky lines to enable it).
> 
>     It's not hacky at all if done properly. For Intel it is simply
>     (Q35_PSEUDO_BUS_PLATFORM << 8) | Q35_PSEUDO_DEVFN_IOAPIC, but it will be
>     0x00a0 (as constant as well) for AMD. So we need some interface to tell
>     those parameters to the IOMMU. Keep in mind that we will need a similar
>     interface for other platform devices, e.g. the HPET.
> 
> 
> In my case IOMMU knows about these parameters but IOAPIC makes interrupt
> requests with a different ID. I thought we should make IOAPIC (and other
> platform devices) make requests using specified IDs.
>  

In fact, intel_iommu knows them as well: The write request comes in via
a well-known address space, the one handed out to the IOMMU. Some for
other platform devices. Associating the ID on write with the requester
would basically mean using the pattern we had in the old iommu code
prior to the introduction of MemTxAttrs. Not sure, though, what is
cleaner here.

Jan

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