Re: [Xen-devel] [v4][PATCH 03/19] xen/vtd: create RMRR mapping
It's a matter of taste to some degree. Unless patches are really involved, I prefer them not to add dead code. Apart from eliminating the case of the code remaining dead (perhaps for extended periods of time) if only parts of a series get applied, it also generally helps review if one can see the consumer of a newly added function right away. FWIW I was thinking the same thing as I was looking at these two patches. Yes, this is our last solution by default. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v4][PATCH 03/19] xen/vtd: create RMRR mapping
Note actually we just need p2m_remove_page() to unmap these mapping on both ept and vt-d sides, and guest_physmap_remove_page is really a wrapper of p2m_remove_page(). And I agree with Tim regarding the desire to avoid code duplication. Yet that's no reason to do it asymmetrically: clear_identity_p2m_entry() could still be an inline (or macro) wrapper around guest_physmap_remove_page(). That way, apart from making I can define that as a macro close to set_identity_p2m_entry() in p2m.h. the code above look nicer, if the latter needs extending in the future for some reason, simply converting the wrapper to a real function is possible without needing to touch the call site(s). This would need to go into patch 2; I wonder whether folding that Yes. and this one wouldn't be warranted, avoiding the former adding Are you saying to fold patch #2 and patch #3? But shouldn't we always define a new and then use that in practice subsequently? Even with two patches, respectively. Thanks Tiejun (at that point) dead code. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v4][PATCH 03/19] xen/vtd: create RMRR mapping
On 24.06.15 at 03:11, tiejun.c...@intel.com wrote: On 2015/6/23 18:12, Jan Beulich wrote: On 23.06.15 at 11:57, tiejun.c...@intel.com wrote: --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1839,7 +1839,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, while ( base_pfn end_pfn ) { -if ( intel_iommu_unmap_page(d, base_pfn) ) +if ( guest_physmap_remove_page(d, base_pfn, base_pfn, 0) ) Yeah, I also thought this may bring some confusions in this context. ret = -ENXIO; base_pfn++; } @@ -1855,8 +1855,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, while ( base_pfn end_pfn ) { -int err = intel_iommu_map_page(d, base_pfn, base_pfn, - IOMMUF_readable|IOMMUF_writable); +int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw); Shouldn't the two continue to be the inverse of one another? Initially, instead of using guest_physmap_remove_page, I was trying to introduce a new, clear_identity_p2m_entry, which can wrapper p2m_remove_page(). But Tim just thought we'd better avoid duplicating code, http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02970.html Maybe guest_physmap_remove_page() does what you want, Note actually we just need p2m_remove_page() to unmap these mapping on both ept and vt-d sides, and guest_physmap_remove_page is really a wrapper of p2m_remove_page(). And I agree with Tim regarding the desire to avoid code duplication. Yet that's no reason to do it asymmetrically: clear_identity_p2m_entry() could still be an inline (or macro) wrapper around guest_physmap_remove_page(). That way, apart from making the code above look nicer, if the latter needs extending in the future for some reason, simply converting the wrapper to a real function is possible without needing to touch the call site(s). This would need to go into patch 2; I wonder whether folding that and this one wouldn't be warranted, avoiding the former adding (at that point) dead code. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v4][PATCH 03/19] xen/vtd: create RMRR mapping
On 23.06.15 at 11:57, tiejun.c...@intel.com wrote: --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1839,7 +1839,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, while ( base_pfn end_pfn ) { -if ( intel_iommu_unmap_page(d, base_pfn) ) +if ( guest_physmap_remove_page(d, base_pfn, base_pfn, 0) ) ret = -ENXIO; base_pfn++; } @@ -1855,8 +1855,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, while ( base_pfn end_pfn ) { -int err = intel_iommu_map_page(d, base_pfn, base_pfn, - IOMMUF_readable|IOMMUF_writable); +int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw); Shouldn't the two continue to be the inverse of one another? Maybe guest_physmap_remove_page() does what you want, but it looks like at least an abuse. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v4][PATCH 03/19] xen/vtd: create RMRR mapping
On 2015/6/23 18:12, Jan Beulich wrote: On 23.06.15 at 11:57, tiejun.c...@intel.com wrote: --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1839,7 +1839,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, while ( base_pfn end_pfn ) { -if ( intel_iommu_unmap_page(d, base_pfn) ) +if ( guest_physmap_remove_page(d, base_pfn, base_pfn, 0) ) Yeah, I also thought this may bring some confusions in this context. ret = -ENXIO; base_pfn++; } @@ -1855,8 +1855,7 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, while ( base_pfn end_pfn ) { -int err = intel_iommu_map_page(d, base_pfn, base_pfn, - IOMMUF_readable|IOMMUF_writable); +int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw); Shouldn't the two continue to be the inverse of one another? Initially, instead of using guest_physmap_remove_page, I was trying to introduce a new, clear_identity_p2m_entry, which can wrapper p2m_remove_page(). But Tim just thought we'd better avoid duplicating code, http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02970.html Maybe guest_physmap_remove_page() does what you want, Note actually we just need p2m_remove_page() to unmap these mapping on both ept and vt-d sides, and guest_physmap_remove_page is really a wrapper of p2m_remove_page(). Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel