Re: [v3][PATCH 02/16] xen/x86/p2m: introduce set_identity_p2m_entry

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

On 2015/6/11 17:23, Jan Beulich wrote:
>>>> On 11.06.15 at 10:23, <tiejun.chen@intel.com> wrote:
>> On 2015/6/11 15:33, Jan Beulich wrote:
>>>>>> On 11.06.15 at 03:15, <tiejun.chen@intel.com> wrote:
>>>> We will create this sort of identity mapping as follows:
>>>>
>>>> If the gfn space is unoccupied, we just set the mapping. If the space
>>>> is already occupied by 1:1 mappings, do nothing. Failed for any
>>>> other cases.
>>>>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>
>>> First of all you continue to be copying each patch to every
>>> maintainer involved in some part of the series. Please limit the
>>
>> I just hope all involved guys can see this series on the whole to
>> review. But,
>>
>>> Cc list of each patch to the actual list of people needing to be
>>> Cc-ed on it (or you know explicitly wanting a copy).
>>
>> Next, I will just send them to each associated maintainer.
>>
>>>
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -898,6 +898,41 @@ int set_mmio_p2m_entry(struct domain *d, unsigned long
>> gfn, mfn_t mfn,
>>>>        return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access);
>>>>    }
>>>>
>>>> +int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
>>>> +                           p2m_access_t p2ma)
>>>> +{
>>>> +    p2m_type_t p2mt;
>>>> +    p2m_access_t a;
>>>> +    mfn_t mfn;
>>>> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>> +    int ret;
>>>> +
>>>> +    if ( paging_mode_translate(p2m->domain) )
>>>> +    {
>>>> +        gfn_lock(p2m, gfn, 0);
>>>> +
>>>> +        mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL);
>>>> +
>>>> +        if ( p2mt == p2m_invalid || mfn_x(mfn) == INVALID_MFN )
>>>
>>> I'm not fundamentally opposed to this extra INVALID_MFN check, but
>>> could you please clarify for which P2M type you saw INVALID_MFN
>>> coming back here, and for which p2m_invalid cases you didn't also
>>> see INVALID_MFN? I.e. I'd really prefer a single check to be used
>>> when that can cover all cases.
>>
>> Actually, I initially adopted "!mfn_valid(mfn)" in our previous version.
>> But Tim issued one comment about this,
>>
>> "I don't think this check is quite right -- for example, this p2m entry
>> might be an MMIO mapping or a PoD entry.  "if ( p2mt == p2m_invalid )"
>> would be better."
>
> Ah, I right, I now remember. In which case checking against
> INVALID_MFN would cover the MMIO case, but not the PoD one.
>
>> But if I just keep his recommended check, you can see the following when
>> I pass through IGD,
>>
>> (XEN) Cannot identity map d1:ad800, already mapped to ffffffffffffffff
>> with p2mt:4.
>>
>> Looks "4" indicates p2m_mmio_dm, right?
>
> And it seems to me that this particular combination would need
> special treatment, i.e. you'd need
>
>         if ( p2mt == p2m_invalid ||
>              (p2mt == p2m_mmio_dm && mfn_x(mfn) == INVALID_MFN) )
>
> as long as p2m_invalid isn't the default type lookups return. But
> I'd recommend waiting for Tim to confirm or further adjust that.
>

Sure.

Thanks
Tiejun