Re: [PATCH v5 05/15] iommu: don't domain_crash() inside iommu_map/unmap_page()

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Tian, Kevin [mailto:kevin.tian@intel.com]
> Sent: 07 August 2018 03:56
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Ian Jackson <Ian.Jackson@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>; Nakajima, Jun <jun.nakajima@intel.com>; George Dunlap
> <George.Dunlap@citrix.com>
> Subject: RE: [PATCH v5 05/15] iommu: don't domain_crash() inside
> iommu_map/unmap_page()
> 
> > From: Paul Durrant [mailto:paul.durrant@citrix.com]
> > Sent: Saturday, August 4, 2018 1:22 AM
> >
> > Turn iommu_map/unmap_page() into straightforward wrappers that check
> > the
> 
> iommu_iotlb_flush is also changed.

Indeed it is. I'll call it out.

> 
> > existence of the relevant iommu_op and call through to it. This makes
> them
> > usable by PV IOMMU code to be delivered in future patches.
> > Leave the decision on whether to invoke domain_crash() up to the caller.
> > This has the added benefit that the (module/line number) message that
> > domain_crash() spits out will be more indicative of where the problem lies.
> >
> > NOTE: This patch includes one bit of clean-up in set_identity_p2m_entry()
> >       replacing use of p2m->domain with the domain pointer passed into the
> >       function.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Reviewed-by: George Dunlap <George.Dunlap@eu.citrix.com>
> > Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one small comment:

Thanks.

> 
> >
> >      if ( need_iommu(p2m->domain) &&
> >           (lpae_valid(orig_pte) || lpae_valid(*entry)) )
> > +    {
> >          rc = iommu_iotlb_flush(p2m->domain, _bfn(gfn_x(sgfn)),
> >                                 1UL << page_order);
> > +        if ( unlikely(rc) && !is_hardware_domain(p2m->domain) )
> > +            domain_crash(p2m->domain);
> > +    }
> 
> to reduce duplication, does it make sense to introduce a wrapper
> like domain_crash_nd ('nd' indicate !is_hardware_domain, and
> becomes a nop for hardware domain)? Then it becomes:
> 
> 	if ( unlikely(rc) )
> 		domain_crash_nd(p2m->domain);

That's a bigger change and I'd like to defer to the other maintainers as to whether they think it is a good idea. I'm happy to change this in v6 if anyone gives me a +1.

  Paul

> 
> Thanks
> Kevin

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