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

[Jan Kiszka]

On 2016-08-02 13:58, David Kiarie wrote:
> 
> 
> On Tue, Aug 2, 2016 at 1:28 PM, Peter Xu <address@hidden
> <mailto:address@hidden>> wrote:
> 
>     On Tue, Aug 02, 2016 at 10:46:13AM +0200, Jan Kiszka 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.
> 
>     Okay.
> 
>     >
>     > >
>     > > I can try to cook another seperate patch to enable it (for 2.8
>     > > possibly?). Thanks for pointing out this issue.
>     >
>     > David needs that IOAPIC ID as well in order to finish interrupt
>     > remapping on AMD. Please synchronize with him who will implement what.
> 
>     Sure. David, so do you like to do it or I cook this patch? :)
> 
> 
> If there are no objections I will look at this employing Jan's approach:
> associating a write with an address space.
>  

That actually means making it an IOMMU-local thing (if write_requester
== invalid -> derive ID from addressed memory region, probably via the
opaque passed to the ops). In that case, everyone could do this on himself.

Jan

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