bogus check in get_page_from_l1e()?

bogus check in get_page_from_l1e()?

[Jan Beulich]

Keir,

in the I/O page code path, we have

        if ( !iomem_access_permitted(pg_owner, mfn, mfn) )
        {
            if ( mfn != (PADDR_MASK >> PAGE_SHIFT) ) /* INVALID_MFN? */
                MEM_LOG("Non-privileged (%u) attempt to map I/O space %08lx", 
                        pg_owner->domain_id, mfn);
            return 0;
        }

What is the reason to suppress the warning for the one specific
(PADDR_MASK >> PAGE_SHIFT) MFN value, i.e. where could this
validly come from and hence warrant not to issue the warning?

Also, the message seems to be having the potential of being
misleading (these days at least, but perhaps it always was), as
it clearly is possible for Dom0 to also be denied a mapping here
(and hence the "Non-privileged" can be wrong).

Bottom line question: Should we issue the warning unconditionally,
just stating the domain ID?

Thanks, Jan

Re: bogus check in get_page_from_l1e()?

[Keir Fraser]

On 08/03/2011 11:07, "Jan Beulich" <JBeulich@novell.com> wrote:

> Keir,
> 
> in the I/O page code path, we have
> 
>         if ( !iomem_access_permitted(pg_owner, mfn, mfn) )
>         {
>             if ( mfn != (PADDR_MASK >> PAGE_SHIFT) ) /* INVALID_MFN? */
>                 MEM_LOG("Non-privileged (%u) attempt to map I/O space %08lx",
>                         pg_owner->domain_id, mfn);
>             return 0;
>         }
> 
> What is the reason to suppress the warning for the one specific
> (PADDR_MASK >> PAGE_SHIFT) MFN value, i.e. where could this
> validly come from and hence warrant not to issue the warning?
> 
> Also, the message seems to be having the potential of being
> misleading (these days at least, but perhaps it always was), as
> it clearly is possible for Dom0 to also be denied a mapping here
> (and hence the "Non-privileged" can be wrong).
> 
> Bottom line question: Should we issue the warning unconditionally,
> just stating the domain ID?

Long time a go, but ISTR that some PV guests (e.g., some versions of our
Linux patches) would attempt to map things during boot that they may not
have access to, and this would manifest as failed attempt to map
INVALID_MFN. Since it's not a genuine real I/O address that is failing to be
mapped, it also seems not so useful to log it.

That said, I'd be open to removing the check if it turns out that these days
failed mappings of INVALID_MFN never 'legitimately' happen. I'm skeptical of
that though.

 -- Keir

> Thanks, Jan
>