Re: [v3][PATCH 00/16] Fix RMRR

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

On 2015/6/11 20:52, Tim Deegan wrote:
> Hi,
>
> At 09:15 +0800 on 11 Jun (1434014109), Tiejun Chen wrote:
>> * Two changes for runtime cycle
>>     patch #2,xen/x86/p2m: introduce set_identity_p2m_entry, on hypervisor side
>>
>>    a>. Introduce paging_mode_translate()
>>    Otherwise, we'll see this error when boot Xen/Dom0
>
> Righto.  Looking at the patch, it would be neater to have
>
>      if ( !paging_mode_translate(p2m->domain) )
>          return 0;
>
> at the start, instead of indenting the whole body of the function in
> an inner scope.

Right.

>
>>    b>. Actually we still need to use "mfn_x(mfn) == INVALID_MFN" to confirm
>>    we're getting an invalid mfn.
>
> Can you give a concrete example of when this is needed?  The code now

Actually we're considering the case that a VM is set with a big memory 
like setting "memory = 2800" [0xaf000000] in the .cfg file, but here 
RMRR is owning such some overlap ranges,

(XEN) [VT-D]dmar.c:808: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:677:   RMRR region: base_addr ac6d3000 end_address 
ac6e6fff
(XEN) [VT-D]dmar.c:808: found ACPI_DMAR_RMRR:
(XEN) [VT-D]dmar.c:677:   RMRR region: base_addr ad800000 end_address 
afffffff

Furthermore, our current policy can help us eliminate this sort of 
conflict. For this example above, the real low memory is limited to 
0xacd3000 finally. So indeed, all RMRR regions aren't treated as RAM, 
and then at least p2m->get_entry() should always return "mfn_x(mfn) == 
INVALID_MFN", right? And,

> looks like this:
>
>         if ( p2mt == p2m_invalid || mfn_x(mfn) == INVALID_MFN )
>             ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>                                 p2m_mmio_direct, p2ma);
>
> which will catch any invalid-mfn mapping, including paged-out pages
> &c.   I suspect the case you care about is the default p2m_mmio_dm type,

based on my understanding,

static mfn_t ept_get_entry(struct p2m_domain *p2m,
                            unsigned long gfn, p2m_type_t *t, 
p2m_access_t* a,
                            p2m_query_t q, unsigned int *page_order)
{
     ...
     mfn_t mfn = _mfn(INVALID_MFN);


     *t = p2m_mmio_dm;
     ...;

     /* This pfn is higher than the highest the p2m map currently holds */
     if ( gfn > p2m->max_mapped_pfn )
         goto out;
     ...

Actually we're falling into this condition so at last, we're always 
getting this combination of (*t = p2m_mmio_dm) and (mfn_t mfn = 
_mfn(INVALID_MFN)).

> which would be better handeld explicitly:
>
>         if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
>             ...
>

So if I'm correct, we should do this check explicitly,

        if ( p2mt == p2m_invalid ||
             (p2mt == p2m_mmio_dm && !mfn_valid(mfn) )

Note this is equivalent to Jan's comment.

Thanks
Tiejun