Re: [Xen-devel] [PATCH v2 03/13] iommu: make use of type-safe BFN and MFN in exported functions

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 10 July 2018 15:29
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: RE: [PATCH v2 03/13] iommu: make use of type-safe BFN and MFN
> in exported functions
> 
> >>> On 10.07.18 at 16:10, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxx]
> >> Sent: 10 July 2018 15:01
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> >> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Konrad
> >> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> >> <sstabellini@xxxxxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Tim
> (Xen.org)
> >> <tim@xxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Jun Nakajima
> >> <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>
> >> Subject: Re: [PATCH v2 03/13] iommu: make use of type-safe BFN and
> MFN
> >> in exported functions
> >>
> >> 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 <paul.durrant@xxxxxxxxxx>
> >> > ---
> >> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> >> > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> >> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >> > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> > Cc: Julien Grall <julien.grall@xxxxxxx>
> >> > Cc: Tim Deegan <tim@xxxxxxx>
> >> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> >> > Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> >> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> >> > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> >> >
> >> > v2:
> >> >  - Addressed comments from Jan.
> >> >  - Use intermediate 'frame' variable to avoid directly encapsulating
> >> >    mfn or gfn values as bfns.
> >>
> >> Explain this one to me?  At the moment I don't see any value from having
> >> the extra variable in the middle.
> >
> > This was something that Jan wanted.
> 
> Ah, yes, it was in a reply to this patch's v1 that I had suggested a
> neutrally named variable in case it can hold values from more than
> one space.

Yes, it was:

" Along the lines of what I've said earlier about mixing address spaces,
this would perhaps not so much need a comment (it's a 1:1 mapping
after all), but rather making more obvious that it's a 1:1 mapping.
This in particular would mean to me to latch page_to_mfn(page) into
a (neutrally named, e.g. "frame") local variable, and use the result in
a way that makes obviously especially on the "map" path that this
really requests a 1:1 mapping. By implication from the 1:1 mapping
it'll then (hopefully) be clear to the reader that which exact name
space is used doesn't really matter."

> In the particular case of _get_page_type(), where I had
> also given the v1 comment, I can't however see that it is now really
> obvious that a 1:1 mapping is being established. To me that would
> mean passing the same local variable (suitably type wrapped where
> needed) twice into iommu_map_page(). It is not clear to me what
> use a mfn_to_gmfn() invocation is inside a is_pv_domain() guarded
> block, now that we don't permit translated PV domains anymore
> (not that I would think that they had worked before the code was
> removed).
> 

I can see about trying to clean up the mfn_to_gmfn() and re-phrase things to 
make it more obvious that the map is 1:1.

  Paul

> Jan
> 


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