Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/7/24 14:14, Jan Beulich wrote:
>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>> On 2014/7/23 23:42, Jan Beulich wrote:
>>>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>>>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
>>>>
>>>>        while ( base_pfn < end_pfn )
>>>>        {
>>>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> +        if ( iommu_use_hap_pt(d) ) {
>>>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>>>> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
>>>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>>>
>>> Do we really need this message, even more so not at guest level?
>>
>> Its useful to debug as I think, but if you insist on this point, I'm
>> fine to remove this as well.
>
> The main question is how frequently this may get printed vs how
> useful the message is.
>
>>> Apart from the above there are several Indentation issues here.
>>
>> Are you saying this thing?
>>
>> if ()
>> {
>> }
>
> Yes, among other things.
>
>> So what about this?
>
> Almost:
>
>> @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
>>
>>        while ( base_pfn < end_pfn )
>>        {
>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +        if ( iommu_use_hap_pt(d) )
>
> Don't you, btw, need to extend this condition by
> && (!iommu_passthrough || !is_hardware_domain(d))?

Why do we need these checks here?

Current problem I met is issued when do GFX passthrough for Windows Guest.

>
>> +        {
>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>
> This still (if you absolutely want to retain the message) needs

I will remove this simply since this is not a big deal :)

> changing to XENLOG_G_DEBUG, and you want to include the domain
> ID in what gets printed for the message to be of any practical use.
>
>> +                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
>
> Additionally please omit the stop at the end. Also, with VTDPREFIX
> not ending with a space, you want the message to be starting
> with one.
>
>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>> +            p2m_lock(p2m);
>> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
>> +                    p2m_mmio_direct, p2m_access_rw) )
>
> Indentation.
>
>> +            {
>> +                p2m_unlock(p2m);
>> +                return -1;
>> +            }
>> +            p2m_unlock(p2m);
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>                                      IOMMUF_readable|IOMMUF_writable) )
>
> Again (here you need you also adjust the second line for indentation
> to match up again).

I'm not familiar with our xen coding style so I'm wondering if we have 
such a similar .pl like checkpatch.pl.

Thanks
Tiejun