On 07/07/2018 12:05 PM, Paul Durrant wrote:
> This patch modifies the declaration of the entry points to the IOMMU
> sub-system to use bfn_t and mfn_t in place of unsigned long. A subsequent
> patch will similarly modify the methods in the iommu_ops structure.
>
> Signed-off-by: Paul Durrant <[email protected]>
> ---
> Cc: Jan Beulich <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Cc: George Dunlap <[email protected]>
> Cc: Ian Jackson <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Stefano Stabellini <[email protected]>
> Cc: Julien Grall <[email protected]>
> Cc: Tim Deegan <[email protected]>
> Cc: Wei Liu <[email protected]>
> Cc: Jun Nakajima <[email protected]>
> Cc: Kevin Tian <[email protected]>
> Cc: George Dunlap <[email protected]>
>
> v2:
> - Addressed comments from Jan.
> - Use intermediate 'frame' variable to avoid directly encapsulating
> mfn or gfn values as bfns.
A couple of comments on particular instances...
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index c53cab44d9..ce12bcff42 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -714,9 +714,12 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long
> gfn_l, unsigned long mfn,
>
> if ( need_iommu(p2m->domain) )
> {
> + unsigned long frame = mfn;
> + bfn_t bfn = _bfn(frame);
> +
> for ( i = 0; i < (1 << page_order); i++ )
> {
> - int ret = iommu_unmap_page(p2m->domain, mfn + i);
> + int ret = iommu_unmap_page(p2m->domain, bfn_add(bfn, i));
Having a 'bfn' variable here makes some sense, because otherwise, if mfn
ever gets the mfn_t type, you'll have
iommu_unmap_page(... _bfn(mfn_x(mfn)+i));
being able to use bfn_add() is much cleaner. I don't think the
intermediate 'frame' variable in the case, really adds anything.
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index d2610e320c..d0926d13e0 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1132,6 +1132,8 @@ map_grant_ref(
> need_iommu = gnttab_need_iommu_mapping(ld);
> if ( need_iommu )
> {
> + unsigned long frame = mfn_x(mfn);
> + bfn_t bfn = _bfn(frame);
> unsigned int kind;
> int err = 0;
>
> @@ -1144,14 +1146,13 @@ map_grant_ref(
> !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
> {
> if ( !(kind & MAPKIND_WRITE) )
> - err = iommu_map_page(ld, mfn_x(mfn), mfn_x(mfn),
> - IOMMUF_readable|IOMMUF_writable);
> + err = iommu_map_page(ld, bfn, mfn,
> + IOMMUF_readable | IOMMUF_writable);
> }
> else if ( act_pin && !old_pin )
> {
> if ( !kind )
> - err = iommu_map_page(ld, mfn_x(mfn), mfn_x(mfn),
> - IOMMUF_readable);
> + err = iommu_map_page(ld, bfn, mfn, IOMMUF_readable);
Here's an example where I think having an extra variable is somewhat
dangerous. Before this change, it's obvious that you have a 1:1
mapping; now, looking just at this line, it's not obvious that bfn ==
mfn. Worse, there's a risk that there will be some sort of bug
introduced which changes bfn, such that bfn != mfn anymore.
If you have to use an intermediate variable here, this should be
iommu_map_page(..., _bfn(frame), _mfn(frame), ...);
But I really think
iommu_map_page(..., _bfn(mfn_x(mfn)), mfn, ...);
makes the most sense here.
-George
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
