On 5/8/19 4:16 PM, Jan Beulich wrote: >>>> On 08.05.19 at 17:08, <george.dun...@citrix.com> 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
