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

[Jan Beulich]

>>> On 10.07.18 at 16:58,  wrote:
> On Tue, Jul 10, 2018 at 3:34 PM, Jan Beulich  wrote:
> On 10.07.18 at 16:29,  wrote:
>>> On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich  wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page,
>>> unsigned long type,
>  struct domain *d = page_get_owner(page);
>  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>  {
> -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
> +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
>
>  if ( (x & PGT_type_mask) == PGT_writable_page )
> -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
> +iommu_ret = iommu_unmap_page(d, bfn);
>  else if ( type == PGT_writable_page )
> -iommu_ret = iommu_map_page(d, gfn_x(gfn),
> -   mfn_x(page_to_mfn(page)),
> +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),

 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.
>>>
>>> I'm sorry, I don't think this is a good idea.
>>>
>>> First of all, it doesn't communicate what you think it does.  What
>>> having an extra variable communicates is, "I am calculating an extra
>>> value that will be used somewhere".  When I saw the "intermediate"
>>> variables all over the place, I didn't immediately think "abstract
>>> space because there's a 1-1 mapping", I was simply confused.
>>>
>>> On the other hand, it is obvious to me that if you 1) have different
>>> kinds of variables (gfn_t, bfn_t, ) and 2) you cast one from the
>>> other doing some math, that you're carefully changing address spaces;
>>> and that if you do _bfn(gfn), that you know you have a 1-1 mapping --
>>> or at least, you very much better well have one, or you're doing
>>> something wrong.
>>
>> Okay - differing opinions, what do you do. To me an expression like
>> _bfn(gfn) looks buggy. And iirc we've had bugs of this kind in the
>> past, which would then contradict your "carefully changing address
>> spaces" assumption.
> 
> To me it looks the same as
>   unsigned long x;
>   char * s;
>   [do something to calculate x]
>   s = (char *)x
> 
> Obviously that sort of casting in C has sharp edges, so you need to be 
> careful.

Right, and I object to casts wherever I can.

> Bugs can happen in any sort of code -- would the bug you have in mind
> actually have been prevented with the use of an extra variable?

How can I tell? Maybe, maybe not.

>> As said in the other reply, something like
>> iommu_map_page(..., _bfn(frame), frame, ...)
>> makes pretty clear that a 1:1 mapping is wanted.
> 
> I just don't see how this is supposed to catch more bugs than
>/* gfns are mapped 1:1 with mfns */
> iommu_map_page(..., _bfn(gfn), gfn, ...)

Well, with the comment it probably doesn't matter how the
variable(s) is/are named.

> There may be some places where having an intermediate variable might
> make things clearer, but there are an awful lot of places in Paul's
> patches where the code just looks like this:
>   unsigned long frame = bfn;
>   gfn_t gfn = _gfn(frame);
>   mfn_t mfn = _mfn(frame);
> 
> Which just seems really pointless.

This indeed looks to be going too far.

Jan


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

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

[George Dunlap]

On Tue, Jul 10, 2018 at 3:34 PM, Jan Beulich  wrote:
 On 10.07.18 at 16:29,  wrote:
>> On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich  wrote:
 --- a/xen/arch/x86/mm.c
 +++ b/xen/arch/x86/mm.c
 @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page,
>> unsigned long type,
  struct domain *d = page_get_owner(page);
  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
  {
 -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
 +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;

  if ( (x & PGT_type_mask) == PGT_writable_page )
 -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
 +iommu_ret = iommu_unmap_page(d, bfn);
  else if ( type == PGT_writable_page )
 -iommu_ret = iommu_map_page(d, gfn_x(gfn),
 -   mfn_x(page_to_mfn(page)),
 +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
>>>
>>> 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.
>>
>> I'm sorry, I don't think this is a good idea.
>>
>> First of all, it doesn't communicate what you think it does.  What
>> having an extra variable communicates is, "I am calculating an extra
>> value that will be used somewhere".  When I saw the "intermediate"
>> variables all over the place, I didn't immediately think "abstract
>> space because there's a 1-1 mapping", I was simply confused.
>>
>> On the other hand, it is obvious to me that if you 1) have different
>> kinds of variables (gfn_t, bfn_t, ) and 2) you cast one from the
>> other doing some math, that you're carefully changing address spaces;
>> and that if you do _bfn(gfn), that you know you have a 1-1 mapping --
>> or at least, you very much better well have one, or you're doing
>> something wrong.
>
> Okay - differing opinions, what do you do. To me an expression like
> _bfn(gfn) looks buggy. And iirc we've had bugs of this kind in the
> past, which would then contradict your "carefully changing address
> spaces" assumption.

To me it looks the same as
  unsigned long x;
  char * s;
  [do something to calculate x]
  s = (char *)x

Obviously that sort of casting in C has sharp edges, so you need to be careful.

Bugs can happen in any sort of code -- would the bug you have in mind
actually have been prevented with the use of an extra variable?

> As said in the other reply, something like
> iommu_map_page(..., _bfn(frame), frame, ...)
> makes pretty clear that a 1:1 mapping is wanted.

I just don't see how this is supposed to catch more bugs than
   /* gfns are mapped 1:1 with mfns */
iommu_map_page(..., _bfn(gfn), gfn, ...)

There may be some places where having an intermediate variable might
make things clearer, but there are an awful lot of places in Paul's
patches where the code just looks like this:
  unsigned long frame = bfn;
  gfn_t gfn = _gfn(frame);
  mfn_t mfn = _mfn(frame);

Which just seems really pointless.

 -George

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

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

[Andrew Cooper]

On 10/07/18 15:34, Jan Beulich wrote:
 On 10.07.18 at 16:29,  wrote:
>> On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich  wrote:
 --- a/xen/arch/x86/mm.c
 +++ b/xen/arch/x86/mm.c
 @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page, 
>> unsigned long type,
  struct domain *d = page_get_owner(page);
  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
  {
 -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
 +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;

  if ( (x & PGT_type_mask) == PGT_writable_page )
 -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
 +iommu_ret = iommu_unmap_page(d, bfn);
  else if ( type == PGT_writable_page )
 -iommu_ret = iommu_map_page(d, gfn_x(gfn),
 -   mfn_x(page_to_mfn(page)),
 +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
>>> 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.
>> I'm sorry, I don't think this is a good idea.
>>
>> First of all, it doesn't communicate what you think it does.  What
>> having an extra variable communicates is, "I am calculating an extra
>> value that will be used somewhere".  When I saw the "intermediate"
>> variables all over the place, I didn't immediately think "abstract
>> space because there's a 1-1 mapping", I was simply confused.
>>
>> On the other hand, it is obvious to me that if you 1) have different
>> kinds of variables (gfn_t, bfn_t, ) and 2) you cast one from the
>> other doing some math, that you're carefully changing address spaces;
>> and that if you do _bfn(gfn), that you know you have a 1-1 mapping --
>> or at least, you very much better well have one, or you're doing
>> something wrong.
> Okay - differing opinions, what do you do. To me an expression like
> _bfn(gfn) looks buggy. And iirc we've had bugs of this kind in the
> past, which would then contradict your "carefully changing address
> spaces" assumption.
>
> As said in the other reply, something like
>   iommu_map_page(..., _bfn(frame), frame, ...)
> makes pretty clear that a 1:1 mapping is wanted.

TBH, I think _bfn(gfn) is better, but any such mixing of address spaces
needs a comment explaining the correctness, even if it is a short /* 1:1
mapping here */

Indirecting through an unsigned long (particularly one with a generic
name) is a misuse of the typesafe interface, because all it does is
serve to confuse the reader.

~Andrew

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

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

[Jan Beulich]

>>> On 10.07.18 at 16:29,  wrote:
> On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich  wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page, 
> unsigned long type,
>>>  struct domain *d = page_get_owner(page);
>>>  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>>>  {
>>> -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
>>> +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
>>>
>>>  if ( (x & PGT_type_mask) == PGT_writable_page )
>>> -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
>>> +iommu_ret = iommu_unmap_page(d, bfn);
>>>  else if ( type == PGT_writable_page )
>>> -iommu_ret = iommu_map_page(d, gfn_x(gfn),
>>> -   mfn_x(page_to_mfn(page)),
>>> +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
>>
>> 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.
> 
> I'm sorry, I don't think this is a good idea.
> 
> First of all, it doesn't communicate what you think it does.  What
> having an extra variable communicates is, "I am calculating an extra
> value that will be used somewhere".  When I saw the "intermediate"
> variables all over the place, I didn't immediately think "abstract
> space because there's a 1-1 mapping", I was simply confused.
> 
> On the other hand, it is obvious to me that if you 1) have different
> kinds of variables (gfn_t, bfn_t, ) and 2) you cast one from the
> other doing some math, that you're carefully changing address spaces;
> and that if you do _bfn(gfn), that you know you have a 1-1 mapping --
> or at least, you very much better well have one, or you're doing
> something wrong.

Okay - differing opinions, what do you do. To me an expression like
_bfn(gfn) looks buggy. And iirc we've had bugs of this kind in the
past, which would then contradict your "carefully changing address
spaces" assumption.

As said in the other reply, something like
iommu_map_page(..., _bfn(frame), frame, ...)
makes pretty clear that a 1:1 mapping is wanted.

Jan



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

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

[George Dunlap]

On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich  wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page, 
>> unsigned long type,
>>  struct domain *d = page_get_owner(page);
>>  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>>  {
>> -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
>> +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
>>
>>  if ( (x & PGT_type_mask) == PGT_writable_page )
>> -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
>> +iommu_ret = iommu_unmap_page(d, bfn);
>>  else if ( type == PGT_writable_page )
>> -iommu_ret = iommu_map_page(d, gfn_x(gfn),
>> -   mfn_x(page_to_mfn(page)),
>> +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
>
> 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.

I'm sorry, I don't think this is a good idea.

First of all, it doesn't communicate what you think it does.  What
having an extra variable communicates is, "I am calculating an extra
value that will be used somewhere".  When I saw the "intermediate"
variables all over the place, I didn't immediately think "abstract
space because there's a 1-1 mapping", I was simply confused.

On the other hand, it is obvious to me that if you 1) have different
kinds of variables (gfn_t, bfn_t, ) and 2) you cast one from the
other doing some math, that you're carefully changing address spaces;
and that if you do _bfn(gfn), that you know you have a 1-1 mapping --
or at least, you very much better well have one, or you're doing
something wrong.

"Documenting" something by introducing random extra unused variables
isn't a good idea.  Either people will waste time trying to verify
that they're not used a different way, or people will become
conditioned to the idea that they're not changing, and will overlook
bugs introduced when the variables actually do change.

 -George

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

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

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 15 March 2018 15:45
> To: Paul Durrant 
> Cc: Julien Grall ; Andrew Cooper
> ; Wei Liu ; George
> Dunlap ; Ian Jackson ;
> Jun Nakajima ; Kevin Tian
> ; Stefano Stabellini ; xen-
> de...@lists.xenproject.org; Konrad Rzeszutek Wilk
> ; Tim (Xen.org) 
> Subject: Re: [PATCH 2/7] iommu: make use of type-safe BFN and MFN in
> exported functions
> 
> >>> On 12.02.18 at 11:47,  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.
> >
> > NOTE: Since (with this patch applied) bfn_t is now in use, the patch also
> >   introduces the 'cscope/grep fodder' to allow the type declaration to
> >   be easily found.
> 
> Ah, here we go. But I continue to think this belong in patch 1.
> 

Ok. I debated it with myself when I wrote the original patches. I'll move the 
relevant hunks.

> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info
> *page, unsigned long type,
> >  struct domain *d = page_get_owner(page);
> >  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> >  {
> > -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
> > +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
> >
> >  if ( (x & PGT_type_mask) == PGT_writable_page )
> > -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
> > +iommu_ret = iommu_unmap_page(d, bfn);
> >  else if ( type == PGT_writable_page )
> > -iommu_ret = iommu_map_page(d, gfn_x(gfn),
> > -   mfn_x(page_to_mfn(page)),
> > +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
> 
> 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.

Ok, I'll re-phrase things in v2.

> 
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -873,12 +873,14 @@ out:
> >  if ( iommu_flags )
> >  for ( i = 0; i < (1 << order); i++ )
> >  {
> > -rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, 
> > iommu_flags);
> > +rc = iommu_map_page(d, _bfn(gfn + i), mfn_add(mfn, i),
> > +iommu_flags);
> >  if ( unlikely(rc) )
> >  {
> >  while ( i-- )
> >  /* If statement to satisfy __must_check. */
> > -if ( iommu_unmap_page(p2m->domain, gfn + i) )
> > +if ( iommu_unmap_page(p2m->domain,
> > +  _bfn(gfn + i)) )
> 
> The fundamental issue of mixed address spaces continues ...
> 

I'll add appropriately an appropriately named stack variable.

> > @@ -781,14 +781,14 @@ guest_physmap_add_entry(struct domain *d,
> gfn_t gfn, mfn_t mfn,
> >  {
> >  for ( i = 0; i < (1 << page_order); i++ )
> >  {
> > -rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
> > -mfn_x(mfn_add(mfn, i)),
> > +rc = iommu_map_page(d, _bfn(mfn_x(mfn) + i),
> > +mfn_add(mfn, i),
> 
> Please check whether some line wrapping can now be avoided, like
> apparently here.
> 

Ok.

> > @@ -1164,7 +1164,9 @@ int set_identity_p2m_entry(struct domain *d,
> unsigned long gfn_l,
> >  {
> >  if ( !need_iommu(d) )
> >  return 0;
> > -return iommu_map_page(d, gfn_l, gfn_l,
> IOMMUF_readable|IOMMUF_writable);
> > +
> > +return iommu_map_page(d, _bfn(gfn_l), _mfn(gfn_l),
> > +  IOMMUF_readable|IOMMUF_writable);
> 
> Please add spaces around | as you touch this (also elsewhere).
> 

Ok.

> > @@ -1254,7 +1256,8 @@ int clear_identity_p2m_entry(struct domain *d,
> unsigned long gfn_l)
> >  {
> > 

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

[Jan Beulich]

>>> On 12.02.18 at 11:47,  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.
> 
> NOTE: Since (with this patch applied) bfn_t is now in use, the patch also
>   introduces the 'cscope/grep fodder' to allow the type declaration to
>   be easily found.

Ah, here we go. But I continue to think this belong in patch 1.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page, 
> unsigned long type,
>  struct domain *d = page_get_owner(page);
>  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>  {
> -gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
> +bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
>  
>  if ( (x & PGT_type_mask) == PGT_writable_page )
> -iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
> +iommu_ret = iommu_unmap_page(d, bfn);
>  else if ( type == PGT_writable_page )
> -iommu_ret = iommu_map_page(d, gfn_x(gfn),
> -   mfn_x(page_to_mfn(page)),
> +iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),

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.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -873,12 +873,14 @@ out:
>  if ( iommu_flags )
>  for ( i = 0; i < (1 << order); i++ )
>  {
> -rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, 
> iommu_flags);
> +rc = iommu_map_page(d, _bfn(gfn + i), mfn_add(mfn, i),
> +iommu_flags);
>  if ( unlikely(rc) )
>  {
>  while ( i-- )
>  /* If statement to satisfy __must_check. */
> -if ( iommu_unmap_page(p2m->domain, gfn + i) )
> +if ( iommu_unmap_page(p2m->domain,
> +  _bfn(gfn + i)) )

The fundamental issue of mixed address spaces continues ...

> @@ -781,14 +781,14 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, 
> mfn_t mfn,
>  {
>  for ( i = 0; i < (1 << page_order); i++ )
>  {
> -rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
> -mfn_x(mfn_add(mfn, i)),
> +rc = iommu_map_page(d, _bfn(mfn_x(mfn) + i),
> +mfn_add(mfn, i),

Please check whether some line wrapping can now be avoided, like
apparently here.

> @@ -1164,7 +1164,9 @@ int set_identity_p2m_entry(struct domain *d, unsigned 
> long gfn_l,
>  {
>  if ( !need_iommu(d) )
>  return 0;
> -return iommu_map_page(d, gfn_l, gfn_l, 
> IOMMUF_readable|IOMMUF_writable);
> +
> +return iommu_map_page(d, _bfn(gfn_l), _mfn(gfn_l),
> +  IOMMUF_readable|IOMMUF_writable);

Please add spaces around | as you touch this (also elsewhere).

> @@ -1254,7 +1256,8 @@ int clear_identity_p2m_entry(struct domain *d, unsigned 
> long gfn_l)
>  {
>  if ( !need_iommu(d) )
>  return 0;
> -return iommu_unmap_page(d, gfn_l);
> +
> +return iommu_unmap_page(d, _bfn(gfn_l));
>  }

No real need for the extra blank line here, as this isn't the main return
point.

Jan

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

[Xen-devel] [PATCH 2/7] iommu: make use of type-safe BFN and MFN in exported functions

[Paul Durrant]

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.

NOTE: Since (with this patch applied) bfn_t is now in use, the patch also
  introduces the 'cscope/grep fodder' to allow the type declaration to
  be easily found.

Signed-off-by: Paul Durrant 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Jun Nakajima 
Cc: Kevin Tian 
Cc: George Dunlap 
---
 xen/arch/arm/p2m.c|  3 ++-
 xen/arch/x86/mm.c |  7 +++
 xen/arch/x86/mm/p2m-ept.c |  8 +---
 xen/arch/x86/mm/p2m-pt.c  |  8 
 xen/arch/x86/mm/p2m.c | 15 +--
 xen/arch/x86/x86_64/mm.c  |  5 +++--
 xen/common/grant_table.c  | 10 ++
 xen/common/memory.c   |  4 ++--
 xen/drivers/passthrough/iommu.c   | 25 -
 xen/drivers/passthrough/vtd/x86/vtd.c |  3 ++-
 xen/include/xen/iommu.h   | 23 +++
 11 files changed, 67 insertions(+), 44 deletions(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 65e8b9c6ea..25e9af6b05 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -957,7 +957,8 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
 
 if ( need_iommu(p2m->domain) &&
  (lpae_valid(orig_pte) || lpae_valid(*entry)) )
-rc = iommu_iotlb_flush(p2m->domain, gfn_x(sgfn), 1UL << page_order);
+rc = iommu_iotlb_flush(p2m->domain, _bfn(gfn_x(sgfn)),
+   1UL << page_order);
 else
 rc = 0;
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 35f204369b..69ce57914b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page, 
unsigned long type,
 struct domain *d = page_get_owner(page);
 if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
 {
-gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
+bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page;
 
 if ( (x & PGT_type_mask) == PGT_writable_page )
-iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
+iommu_ret = iommu_unmap_page(d, bfn);
 else if ( type == PGT_writable_page )
-iommu_ret = iommu_map_page(d, gfn_x(gfn),
-   mfn_x(page_to_mfn(page)),
+iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
IOMMUF_readable|IOMMUF_writable);
 }
 }
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 66dbb3e83a..e1ebd25e57 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -873,12 +873,14 @@ out:
 if ( iommu_flags )
 for ( i = 0; i < (1 << order); i++ )
 {
-rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, 
iommu_flags);
+rc = iommu_map_page(d, _bfn(gfn + i), mfn_add(mfn, i),
+iommu_flags);
 if ( unlikely(rc) )
 {
 while ( i-- )
 /* If statement to satisfy __must_check. */
-if ( iommu_unmap_page(p2m->domain, gfn + i) )
+if ( iommu_unmap_page(p2m->domain,
+  _bfn(gfn + i)) )
 continue;
 
 break;
@@ -887,7 +889,7 @@ out:
 else
 for ( i = 0; i < (1 << order); i++ )
 {
-ret = iommu_unmap_page(d, gfn + i);
+ret = iommu_unmap_page(d, _bfn(gfn + i));
 if ( !rc )
 rc = ret;
 }
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index ad6f9ef10d..0e6392a959 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -696,13 +696,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, 
mfn_t mfn,
 else if ( iommu_pte_flags )
 for ( i = 0; i < (1UL << page_order); i++ )
 {
-rc = iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
-iommu_pte_flags);
+rc =