Re: [Xen-devel] [PATCH] x86/mm: subsume set_gpfn_from_mfn() into guest_physmap_add_entry()

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 5/8/19 4:16 PM, Jan Beulich wrote:
>>>> On 08.05.19 at 17:08, <george.dunlap@xxxxxxxxxx> wrote:
>> On 5/2/19 7:58 AM, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -841,15 +841,19 @@ guest_physmap_add_entry(struct domain *d
>>>           * any guest-requested type changes succeed and remove the IOMMU
>>>           * entry).
>>>           */
>>> -        if ( !need_iommu_pt_sync(d) || t != p2m_ram_rw )
>>> +        if ( t != p2m_ram_rw )
>>>              return 0;
>>
>> So, you seem to be claiming that the only way to get here is via
>> guest_physmap_add_page(),
> 
> Well, I'm not "claiming" anything here, I'm just modifying existing
> code (and no more than what fits under this patch's title).

Not here, but in the other email.

But looking at it -- it looks like on x86, guest_physmap_add_entry() is
actually called from *exactly* two locations:
hvm/grant_table.c:create_grant_p2m_mapping(), and
p2m.h:guest_physmap_add_page().

Which sort of makes me wonder if it might not be better to add the PV
conditional to guest_physmap_add_page() instead, and leave
guest_physmap_add_entry() as entirely HVM.

> 
>> which will always call this function with
>> p2m_ram_rw.  So then what's the point of this conditional at all
>> anymore?  Would it be better to add an ASSERT(t == p2m_ram_rw) here?
>>
>> And if we ever *do* get here with t == p2m_ram_rw, do we really not want
>> to call set_gpfn_from_mfn()?
> 
> Thinking about e.g. p2m_grant_map_* I wouldn't want to add the
> suggested ASSERT(), and the M2P doesn't want updating in that
> case either.

Sorry, do you think p2m_grant_map_* is more likely somehow than
p2m_ram_ro?  It looks very much like neither one should ever happen. The
purpose of having an ASSERT() there is to alert developers making such a
fundamental change to the fact that they need to think carefully about
what should happen in that case.

(For safety sake, because ASSERT is only a debugging aid, we certainly
need to keep some sort of conditional here to make sure non-writable
pages don't end up with writable mappings in the IOMMU.)

If we don't have an ASSERT(), then we need to think carefully about
whether we should call set_gp2n_from_mfn() if t is, for instance,
p2m_ram_ro.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel