Re: [Qemu-devel] [PULL 08/41] intel_iommu: support device iotlb descriptor

[Jason Wang <jasowang@redhat.com>]

On 2017年01月19日 11:28, Peter Xu wrote:
> On Thu, Jan 19, 2017 at 10:50:59AM +0800, Jason Wang wrote:
>
> [...]
>
>>>> +static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>>>> +                                          VTDInvDesc *inv_desc)
>>>> +{
>>>> +    VTDAddressSpace *vtd_dev_as;
>>>> +    IOMMUTLBEntry entry;
>>>> +    struct VTDBus *vtd_bus;
>>>> +    hwaddr addr;
>>>> +    uint64_t sz;
>>>> +    uint16_t sid;
>>>> +    uint8_t devfn;
>>>> +    bool size;
>>>> +    uint8_t bus_num;
>>>> +
>>>> +    addr = VTD_INV_DESC_DEVICE_IOTLB_ADDR(inv_desc->hi);
>>>> +    sid = VTD_INV_DESC_DEVICE_IOTLB_SID(inv_desc->lo);
>>>> +    devfn = sid & 0xff;
>>>> +    bus_num = sid >> 8;
>>>> +    size = VTD_INV_DESC_DEVICE_IOTLB_SIZE(inv_desc->hi);
>>>> +
>>>> +    if ((inv_desc->lo & VTD_INV_DESC_DEVICE_IOTLB_RSVD_LO) ||
>>>> +        (inv_desc->hi & VTD_INV_DESC_DEVICE_IOTLB_RSVD_HI)) {
>>>> +        VTD_DPRINTF(GENERAL, "error: non-zero reserved field in Device "
>>>> +                    "IOTLB Invalidate Descriptor hi 0x%"PRIx64 " lo 0x%"PRIx64,
>>>> +                    inv_desc->hi, inv_desc->lo);
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    vtd_bus = vtd_find_as_from_bus_num(s, bus_num);
>>>> +    if (!vtd_bus) {
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    vtd_dev_as = vtd_bus->dev_as[devfn];
>>>> +    if (!vtd_dev_as) {
>>>> +        goto done;
>>>> +    }
>>>> +
>>>> +    if (size) {
>>>> +        sz = 1 << (ctz64(~(addr | (VTD_PAGE_MASK_4K - 1))) + 1);
>>> This should be 1ULL.
>> Yes.
>>
>>>    It could also be converted to cto64:
>>>
>>> (VTD_PAGE_SIZE * 2) << cto64(addr >> VTD_PAGE_SHIFT)
>>>
>>> Here, I'm shifting addr right to avoid the case of an addr that is all ones.
>>>
>>> It probably could use a comment too. :)  The examples in table 2-4 of
>>> the PCIe ATS specification are useful:
>>>
>>> 	S = 0, bits 15:12 = xxxx     range size: 4K
>>> 	S = 1, bits 15:12 = xxx0     range size: 8K
>>> 	S = 1, bits 15:12 = xx01     range size: 16K
>>> 	S = 1, bits 15:12 = x011     range size: 32K
>>> 	S = 1, bits 15:12 = 0111     range size: 64K
>>>
>>>          and so on
>> This seems simpler let me add them comment and convert it to cto64.
>>
>>>> +        addr &= ~(sz - 1);
> [1]
>
>>>> +    } else {
>>>> +        sz = VTD_PAGE_SIZE;
>>>> +    }
>>>> +    entry.target_as = &vtd_dev_as->as;
>>>> +    entry.addr_mask = sz - 1;
>>>> +    entry.iova = addr;
>>> If S=1, entry.iova must mask away the 1 bits that specified the size.
>>> For example,
>>>
>>>       addr = 0xabcd1000
>>>
>>> has cto64(0xabcd1) == 1, so it indicates a 16K invalidation from
>>> 0xabcd0000 to 0xabcd3fff.  The "1" must be masked away with "addr & -sz"
>>> or "addr & ~entry.addr_mask".
>>>
>>> Thanks,
>>>
>>> Paolo
>> Good catch.
>>
>> Let me fix it.
> Is above [1] doing that?
>
> Thanks,
>
> -- peterx

Yes, speak too fast :(

Thanks