[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.