HYBRID: max_mapped_pfn in ept_set_entry()

HYBRID: max_mapped_pfn in ept_set_entry()

[Mukesh Rathor]

Hi guys,

I've a question. For my hybrid (PV in HVM) dom0 I do 1-1 mapping for 
mmio pages.  So, gpfn 000fbf23 maps to 000fbf23 in EPT. The
max_mapped_pfn is not adjusted in ept_set_entry() because the mfn is
not valid: 

    if ( mfn_valid(mfn_x(mfn)) &&
         (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
         p2m->max_mapped_pfn = gfn + (1UL << order) - 1;

As a result, clear_mmio_p2m_entry() fails because ept_get_entry() fails
because:
    if ( gfn > p2m->max_mapped_pfn )
            goto out;


I'm trying to figure the right thing to do here. Should I just change
the "gfn > p2m->max_mapped_pfn" in ept_get_entry() to check for
INVALID_MFN? I really shouldn't be adjusting max_mapped_pfn for MMIO
pages, right?

thanks,
Mukesh

Re: HYBRID: max_mapped_pfn in ept_set_entry()

[Ian Campbell]

On Thu, 2012-03-29 at 01:08 +0100, Mukesh Rathor wrote:
> Hi guys,
> 
> I've a question. For my hybrid (PV in HVM) dom0 I do 1-1 mapping for 
> mmio pages.  So, gpfn 000fbf23 maps to 000fbf23 in EPT. The
> max_mapped_pfn is not adjusted in ept_set_entry() because the mfn is
> not valid: 
> 
>     if ( mfn_valid(mfn_x(mfn)) &&
>          (gfn + (1UL << order) - 1 > p2m->max_mapped_pfn) )
>          p2m->max_mapped_pfn = gfn + (1UL << order) - 1;
> 
> As a result, clear_mmio_p2m_entry() fails because ept_get_entry() fails
> because:
>     if ( gfn > p2m->max_mapped_pfn )
>             goto out;
> 
> 
> I'm trying to figure the right thing to do here. Should I just change
> the "gfn > p2m->max_mapped_pfn" in ept_get_entry() to check for
> INVALID_MFN? I really shouldn't be adjusting max_mapped_pfn for MMIO
> pages, right?

I'm no expert on the p2m side of things but ept_get_entry says:
    /* This pfn is higher than the highest the p2m map currently holds */
    if ( gfn > p2m->max_mapped_pfn )
        goto out;

which suggests to me that this is just an optimisation (skipping a
lookup which can never succeed) and therefore it is appropriate to
update max_mapped_pfn. After all a 1-1 mapped pfn is still a pfn.

Other places, like ept_walk_table seem to make a similar optimisation. 

Unless there is some reason to assume that these functions will never be
passed an MMIO pfn?

The only user of that var which looks like it might need some thought in
this context would be domain_get_maximum_gpfn and its callers.

Ian.

Re: HYBRID: max_mapped_pfn in ept_set_entry()

[Tim Deegan]

At 10:29 +0100 on 29 Mar (1333016954), Ian Campbell wrote:
> On Thu, 2012-03-29 at 01:08 +0100, Mukesh Rathor wrote:
> > I'm trying to figure the right thing to do here. Should I just change
> > the "gfn > p2m->max_mapped_pfn" in ept_get_entry() to check for
> > INVALID_MFN? I really shouldn't be adjusting max_mapped_pfn for MMIO
> > pages, right?
> 
> I'm no expert on the p2m side of things but ept_get_entry says:
>     /* This pfn is higher than the highest the p2m map currently holds */
>     if ( gfn > p2m->max_mapped_pfn )
>         goto out;
> 
> which suggests to me that this is just an optimisation (skipping a
> lookup which can never succeed) and therefore it is appropriate to
> update max_mapped_pfn. After all a 1-1 mapped pfn is still a pfn.

Yes, you should adjust max_mapped_pfn, in ept_set_entry() and in
p2m_set_entry() (p2m-pt.c) so we don't get the same bug reappearing
on AMD.  Instead of checking mfn_valid(), they should check 
(p2mt != p2m_invalid && p2mt != p2m_mmio_dm).

> Other places, like ept_walk_table seem to make a similar optimisation. 
> 
> Unless there is some reason to assume that these functions will never be
> passed an MMIO pfn?

No - it's just that this check predates anything other than RAM and
emulated MMIO.

Tim.