On Wed, Aug 10, 2016 at 8:49 AM, Peter Xu <pet...@redhat.com> wrote:
> On Tue, Aug 09, 2016 at 05:32:17PM +0300, David Kiarie wrote:
>
> [...]
>
> > @@ -2252,14 +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;
> > + VTDAddressSpace *as = opaque;
> > + uint16_t sid = pci_bus_num(as->bus) << 8 | as->devfn;
>
> SID can be something not equals to BDF. E.g., when there are PCI
> bridges. See pci_requester_id(). However...
>
> >
> > from.address = (uint64_t) addr + VTD_INTERRUPT_ADDR_FIRST;
> > from.data = (uint32_t) value;
> >
> > - if (!attrs.unspecified) {
> > - /* We have explicit Source ID */
> > - sid = attrs.requester_id;
> > + if (attrs.requester_id != sid) {
> > + VTD_DPRINTF(GENERAL, "int remap request for sid 0x%04x"
> > + " requester_id 0x%04x couldn't be verified",
> > + sid, attrs.requester_id);
> > + return MEMTX_ERROR;
>
> ...I am not sure whether we need extra check here. In what case will
> attrs.requester_id != sid ?
>
Meaning I should remove this check ?
>
> Though I agree to remove the original if().
>
> > }
> >
> > ret = vtd_interrupt_remap_msi(opaque, &from, &to, sid);
> > @@ -2325,7 +2326,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState
> *s, PCIBus *bus, int devfn)
> > memory_region_init_iommu(&vtd_dev_as->iommu, OBJECT(s),
> > &s->iommu_ops, "intel_iommu",
> UINT64_MAX);
> > memory_region_init_io(&vtd_dev_as->iommu_ir, OBJECT(s),
> > - &vtd_mem_ir_ops, s, "intel_iommu_ir",
> > + &vtd_mem_ir_ops, vtd_dev_as,
> "intel_iommu_ir",
> > VTD_INTERRUPT_ADDR_SIZE);
> > memory_region_add_subregion(&vtd_dev_as->iommu,
> VTD_INTERRUPT_ADDR_FIRST,
> > &vtd_dev_as->iommu_ir);
> > @@ -2465,6 +2466,9 @@ static void vtd_realize(DeviceState *dev, Error
> **errp)
> > vtd_init(s);
> > sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, Q35_HOST_BRIDGE_IOMMU_ADDR);
> > pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
> > + /* IOMMU expected IOAPIC SID */
> > + x86_iommu->ioapic_bdf = Q35_PSEUDO_DEVFN_IOAPIC << 8 |
> > + Q35_PSEUDO_DEVFN_IOAPIC;
>
> We can use PCI_BUILD_BDF() here.
>
> -- peterx
>
