[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset |
Date: |
Sat, 16 Aug 2014 10:47:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
On 2014-08-16 10:45, Jan Kiszka wrote:
> On 2014-08-16 09:54, Knut Omang wrote:
>> On Fri, 2014-08-15 at 19:37 +0800, Le Tan wrote:
>>> Hi Knut,
>>>
>>> 2014-08-15 19:15 GMT+08:00 Knut Omang <address@hidden>:
>>>> On Fri, 2014-08-15 at 06:42 +0200, Knut Omang wrote:
>>>>> On Thu, 2014-08-14 at 14:10 +0200, Jan Kiszka wrote:
>>>>>> On 2014-08-14 13:15, Michael S. Tsirkin wrote:
>>>>>>> On Mon, Aug 11, 2014 at 03:04:57PM +0800, Le Tan wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> These patches are intended to introduce Intel IOMMU (VT-d) emulation
>>>>>>>> to q35
>>>>>>>> chipset. The major job in these patches is to add support for
>>>>>>>> emulating Intel
>>>>>>>> IOMMU according to the VT-d specification, including basic responses
>>>>>>>> to CSRs
>>>>>>>> accesses, the logics of DMAR (DMA remapping) and DMA memory address
>>>>>>>> translations.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Looks very good overall, I noted some coding style issues - I didn't
>>>>>>> bother reporting each issue in every place where it appears - reported
>>>>>>> each issue once only, so please find and fix all instances of each
>>>>>>> issue.
>>>>>>
>>>>>> BTW, because I was in urgent need for virtual test environment for
>>>>>> Jailhouse, I hacked interrupt remapping on top of Le's patches:
>>>>>>
>>>>>> http://git.kiszka.org/?p=qemu.git;a=shortlog;h=refs/heads/queues/vtd-intremap
>>>>>>
>>>>>> The approach likely needs further discussions and refinements but it
>>>>>> already allows me to work on top with our hypervisor, and also Linux.
>>>>>> You can see from the last commit that Le's work made it pretty easy to
>>>>>> build this on top.
>>>>>
>>>>> Le,
>>>>>
>>>>> I have tried Jan's branch with my device setup which consists of a
>>>>> minimal q35 setup, an ioh3420 root port (specified as -device
>>>>> ioh3420,slot=0 ) and a pcie device plugged into that root port, which
>>>>> gives the following lscpi -t:
>>>>>
>>>>> -[0000:00]-+-00.0
>>>>> +-01.0
>>>>> +-02.0
>>>>> +-03.0-[01]----00.0
>>>>> +-04.0
>>>>> +-1f.0
>>>>> +-1f.2
>>>>> \-1f.3
>>>>>
>>>>> All seems to work beautifully (I see the ISA bridge happily receive
>>>>> translations) until the first DMA from my device model (at 1:00.0)
>>>>> arrives, at which point I get:
>>>>>
>>>>> [ 1663.732413] dmar: DMAR:[DMA Write] Request device [00:03.0] fault addr
>>>>> fffa0000
>>>>> [ 1663.732413] DMAR:[fault reason 02] Present bit in context entry is
>>>>> clear
>>>>>
>>>>> I would have expected request device 01:00.0 for this.
>>>>> It is not clear to me yet if this is a weakness of the implementation of
>>>>> ioh3420 or the iommu. Just wanted to let you know right away in case you
>>>>> can shed some light to it or it is an easy fix,
>>>>>
>>>>> The device uses pci_dma_rw with itself as device pointer.
>>>>
>>>> To verify my hypothesis: with this rude hack my device now works much
>>>> better:
>>>>
>>>> @@ -774,6 +780,8 @@ static void iommu_translate(VTDAddressSpace *vtd_as,
>>>> int bus_num, int devfn,
>>>> is_fpd_set = ce.lo & VTD_CONTEXT_ENTRY_FPD;
>>>> } else {
>>>> ret_fr = dev_to_context_entry(s, bus_num, devfn, &ce);
>>>> + if (ret_fr)
>>>> + ret_fr = dev_to_context_entry(s, 1, 0, &ce);
>>>> is_fpd_set = ce.lo & VTD_CONTEXT_ENTRY_FPD;
>>>> if (ret_fr) {
>>>> ret_fr = -ret_fr;
>>>>
>>>> Looking at how things look on hardware, multiple devices often receive
>>>> overlapping DMA address ranges for different physical addresses.
>>>>
>>>> So if I understand the way this works, every requester ID would also
>>>> need to have it's own unique VTDAddressSpace, as each pci
>>>> device/function sees a unique DMA address space..
>>>
>>> ioh3420 is a pcie-to-pcie bridge, right?
>>
>> Yes.
>>
>>> In my opinion, each pci-e
>>> device behind the pcie-to-pcie bridge can be assigned individually.
>>> For now I added the VT-d to q35 by just adding it to the root pci bus.
>>> You can see here in q35.c:
>>> pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
>>> So if we add a pcie-to-pcie bridge, we may have to call the
>>> pci_setup_iommu() for that new bus. I don't know where to hook into
>>> this now. :) If you know the mechanism behind that, you can try to add
>>> that for the new bus. (I will dive into this after the clean up.)
>>> What do you think?
>>
>> Thanks for the quick answer, that helped a lot!
>>
>> Looking into the details here I realize it is slightly more complicated:
>> secondary buses are enumerated after device instantiation, as part of
>> the host PCI enumeration, so if I add a similar setup call in the bridge
>> setup, it will be called for a new device long before it has received
>> it's bus number from the OS (via config[PCI_SECONDARY_BUS] )
>>
>> I agree that the lookup function for contexts needs to be as efficient
>> as possible so the simple <busno,defvn> lookup key may be the best
>> solution but then the address_spaces table cannot be populated with the
>> secondary bus entries before it receives a nonzero != 255 bus number,
>> eg. along the lines of this:
>>
>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>> index 4becdc1..d9a8c23 100644
>> --- a/hw/pci/pci_bridge.c
>> +++ b/hw/pci/pci_bridge.c
>> @@ -265,6 +265,12 @@ void pci_bridge_write_config(PCIDevice *d,
>> pci_bridge_update_mappings(s);
>> }
>>
>> + if (ranges_overlap(address, len, PCI_SECONDARY_BUS, 1)) {
>> + int bus_num = pci_bus_num(&s->sec_bus);
>> + if (bus_num != 0xff && bus_num != 0x00)
>> + <handle bus number change>
>> + }
>> +
>> newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
>> if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
>> /* Trigger hot reset on 0->1 transition. */
>>
>> but it is getting complicated...
>> Thoughts?
>
> Point to the PCI bus from VTDAddressSpace instead of storing the bus_num
> there?
Also, each PCIe bus should hold an array of VTDAddressSpaces, instead of
the IntelIOMMUState.
Jan
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 3/5] intel-iommu: add DMAR table to ACPI tables, (continued)
- [Qemu-devel] [PATCH v3 5/5] intel-iommu: add supports for queued invalidation interface, Le Tan, 2014/08/11
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Michael S. Tsirkin, 2014/08/14
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Jan Kiszka, 2014/08/14
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Knut Omang, 2014/08/15
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Knut Omang, 2014/08/15
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Le Tan, 2014/08/15
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Knut Omang, 2014/08/16
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Jan Kiszka, 2014/08/16
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset,
Jan Kiszka <=
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Knut Omang, 2014/08/18
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Jan Kiszka, 2014/08/18
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Knut Omang, 2014/08/19
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Knut Omang, 2014/08/19
- Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset, Jan Kiszka, 2014/08/19