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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.06.15 at 10:23, <tiejun.chen@xxxxxxxxx> wrote:
> On 2015/6/11 15:33, Jan Beulich wrote:
>>>>> On 11.06.15 at 03:15, <tiejun.chen@xxxxxxxxx> 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@xxxxxxxxx>
>>
>> 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.

Jan


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