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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.19 at 17:45, <george.dunlap@xxxxxxxxxx> wrote:
> 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.

Yes, I think I'll do this.

>>> 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.

Let's face it - p2m types are a HVM concept only anyway. But this
discussion becomes moot (afaict) with the change above anyway.

Jan



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