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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.07.14 at 03:23, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/7/23 23:42, Jan Beulich wrote:
>>>>> On 23.07.14 at 11:35, <tiejun.chen@xxxxxxxxx> 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 coding style 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))?

> +        {
> +            dprintk(XENLOG_DEBUG VTDPREFIX,

This still (if you absolutely want to retain the message) needs
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).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel