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

[Peter Xu]

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? :)

Thanks,

-- peterx