Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[George Dunlap]

On Fri, Oct 2, 2015 at 8:31 AM, Jan Beulich  wrote:
 George Dunlap  10/01/15 6:16 PM >>>
>>On 01/10/15 11:25, Jan Beulich wrote:
>>> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>>>  is an apparent inconsistency with PoD handling: 2M mappings get
>>>  valid entries created, while 4k mappings don't. It would seem to
>>>  me that the 4k case needs changing, even if today this may only
>>>  be a latent bug. Question of course is why we don't rely on
>>>  p2m_type_to_flags() doing its job properly and instead special
>>>  case certain P2M types.
>>
>>The inconsistency in the conditionals there is a bit strange; but I'm
>>pretty sure that in the 2MB case it is (at the moment) superfluous,
>>because at the moment it seems that when setting a page with type
>>p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>>
>>(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>>some point without comment.)
>
> Perhaps just because the magic MFN didn't always work? Tim?
> To me it looks wrong to pass anything other than INVALID_MFN
> there.

Well the magic MFN was also a valid MFN (it was like 1<<9 or
something), which is probably worse than _mfn(0). :-)

If we change it to INVALID_MFN, we'd have to either add exceptions in
all of those conditionals (so it doesn't get l1e_empty()), or perhaps
(as you say) trust p2m_type_to_flags() to DTRT.  The second is
probably the best option, but obviously not at this stage in the
release.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Jan Beulich]

>>> Wei Liu  10/02/15 11:16 AM >>>
>I think George and you are talking about another function?  Is there
>anything that prevents this patch from being committed as-is?

Nothing technical. Just me getting into the office (early afternoon I guess).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Wei Liu]

On Fri, Oct 02, 2015 at 01:31:37AM -0600, Jan Beulich wrote:
> >>> George Dunlap  10/01/15 6:16 PM >>>
> >On 01/10/15 11:25, Jan Beulich wrote:
> >> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
> >>  is an apparent inconsistency with PoD handling: 2M mappings get
> >>  valid entries created, while 4k mappings don't. It would seem to
> >>  me that the 4k case needs changing, even if today this may only
> >>  be a latent bug. Question of course is why we don't rely on
> >>  p2m_type_to_flags() doing its job properly and instead special
> >>  case certain P2M types.
> >
> >The inconsistency in the conditionals there is a bit strange; but I'm
> >pretty sure that in the 2MB case it is (at the moment) superfluous,
> >because at the moment it seems that when setting a page with type
> >p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
> >
> >(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
> >some point without comment.)
> 
> Perhaps just because the magic MFN didn't always work? Tim?
> To me it looks wrong to pass anything other than INVALID_MFN
> there.
> 

I think George and you are talking about another function?  Is there
anything that prevents this patch from being committed as-is?

Wei.

> Jan
> 
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[George Dunlap]

On Fri, Oct 2, 2015 at 10:16 AM, Wei Liu  wrote:
> On Fri, Oct 02, 2015 at 01:31:37AM -0600, Jan Beulich wrote:
>> >>> George Dunlap  10/01/15 6:16 PM >>>
>> >On 01/10/15 11:25, Jan Beulich wrote:
>> >> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>> >>  is an apparent inconsistency with PoD handling: 2M mappings get
>> >>  valid entries created, while 4k mappings don't. It would seem to
>> >>  me that the 4k case needs changing, even if today this may only
>> >>  be a latent bug. Question of course is why we don't rely on
>> >>  p2m_type_to_flags() doing its job properly and instead special
>> >>  case certain P2M types.
>> >
>> >The inconsistency in the conditionals there is a bit strange; but I'm
>> >pretty sure that in the 2MB case it is (at the moment) superfluous,
>> >because at the moment it seems that when setting a page with type
>> >p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>> >
>> >(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>> >some point without comment.)
>>
>> Perhaps just because the magic MFN didn't always work? Tim?
>> To me it looks wrong to pass anything other than INVALID_MFN
>> there.
>>
>
> I think George and you are talking about another function?  Is there
> anything that prevents this patch from being committed as-is?

No -- I'm just answering Jan's "To Be Done" comment (I assume that's
what TBD means).  He's noticed something strange; but it's been there
for quite a while, and so both by inspection and long testing
(probably a few releases now) works.  No point fiddling with it until
after the release.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Tim Deegan]

At 01:31 -0600 on 02 Oct (1443749497), Jan Beulich wrote:
> >>> George Dunlap  10/01/15 6:16 PM >>>
> >On 01/10/15 11:25, Jan Beulich wrote:
> >> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
> >>  is an apparent inconsistency with PoD handling: 2M mappings get
> >>  valid entries created, while 4k mappings don't. It would seem to
> >>  me that the 4k case needs changing, even if today this may only
> >>  be a latent bug. Question of course is why we don't rely on
> >>  p2m_type_to_flags() doing its job properly and instead special
> >>  case certain P2M types.
> >
> >The inconsistency in the conditionals there is a bit strange; but I'm
> >pretty sure that in the 2MB case it is (at the moment) superfluous,
> >because at the moment it seems that when setting a page with type
> >p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
> >
> >(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
> >some point without comment.)
> 
> Perhaps just because the magic MFN didn't always work? Tim?
> To me it looks wrong to pass anything other than INVALID_MFN
> there.

Hmm, I really didn't explain that at all, did I? :(  ISTR some more
discussion about this, but it doesn't seem to have been on the
list.

I suspect I wanted to change it to INVALID_MFN but got tangled up in
code that treated that as an error without checking the p2m type --
the p2m type stuff was not in use everywhere at that point.

I'd agree that using P2M_INVALID and fixing any confused callers is
the right thing to do.

Cheers,

Tim.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Jan Beulich]

>>> On 02.10.15 at 11:47,  wrote:
> On Fri, Oct 2, 2015 at 8:31 AM, Jan Beulich  wrote:
> George Dunlap  10/01/15 6:16 PM >>>
>>>On 01/10/15 11:25, Jan Beulich wrote:
 TBD: As already mentioned on the large-page-MMIO-mapping patch, there
  is an apparent inconsistency with PoD handling: 2M mappings get
  valid entries created, while 4k mappings don't. It would seem to
  me that the 4k case needs changing, even if today this may only
  be a latent bug. Question of course is why we don't rely on
  p2m_type_to_flags() doing its job properly and instead special
  case certain P2M types.
>>>
>>>The inconsistency in the conditionals there is a bit strange; but I'm
>>>pretty sure that in the 2MB case it is (at the moment) superfluous,
>>>because at the moment it seems that when setting a page with type
>>>p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>>>
>>>(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>>>some point without comment.)
>>
>> Perhaps just because the magic MFN didn't always work? Tim?
>> To me it looks wrong to pass anything other than INVALID_MFN
>> there.
> 
> Well the magic MFN was also a valid MFN (it was like 1<<9 or
> something), which is probably worse than _mfn(0). :-)
> 
> If we change it to INVALID_MFN, we'd have to either add exceptions in
> all of those conditionals (so it doesn't get l1e_empty()), or perhaps
> (as you say) trust p2m_type_to_flags() to DTRT.  The second is
> probably the best option, but obviously not at this stage in the
> release.

Oh, of course I meant such a change to go into unstable only. Not
the least because surely there is a risk for regressions there.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Jan Beulich]

>>> George Dunlap  10/01/15 6:16 PM >>>
>On 01/10/15 11:25, Jan Beulich wrote:
>> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>>  is an apparent inconsistency with PoD handling: 2M mappings get
>>  valid entries created, while 4k mappings don't. It would seem to
>>  me that the 4k case needs changing, even if today this may only
>>  be a latent bug. Question of course is why we don't rely on
>>  p2m_type_to_flags() doing its job properly and instead special
>>  case certain P2M types.
>
>The inconsistency in the conditionals there is a bit strange; but I'm
>pretty sure that in the 2MB case it is (at the moment) superfluous,
>because at the moment it seems that when setting a page with type
>p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.
>
>(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
>some point without comment.)

Perhaps just because the magic MFN didn't always work? Tim?
To me it looks wrong to pass anything other than INVALID_MFN
there.

Jan




___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Jan Beulich]

>>> On 01.10.15 at 15:34,  wrote:
> On 01/10/15 11:25, Jan Beulich wrote:
>> @@ -645,11 +665,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>>   && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
>>  p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
>>  
>> -if ( iommu_enabled && need_iommu(p2m->domain) )
>> +if ( iommu_enabled && need_iommu(p2m->domain) &&
>> + (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
>>  {
>>  if ( iommu_use_hap_pt(p2m->domain) )
>>  {
>> -if ( old_mfn && (old_mfn != mfn_x(mfn)) )
>> +if ( iommu_old_flags )
>>  amd_iommu_flush_pages(p2m->domain, gfn, page_order);
> 
> iommu_hap_pt_share is hardwired to 0 on AMD, making this if() clause
> effectively dead.  Is this what you mean by your second TBD?  I would
> suggest dropping it.

Yes, that's what I mean.

>>  }
>>  else
>>
>>
> 
> In this else clause there is a now-shadowed "flags" which might better
> be renamed to iommu_flags to avoid confusion.
> 
> There is also an extra shadowed 'i' which could do with removing, as it
> introduces a 64bit->32bit truncation (which is not currently a problem).

See the (not re-submitted because it didn't really change) follow-up
patch
(http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg02585.html)
which deletes that pointless variable altogether.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Andrew Cooper]

On 01/10/15 11:25, Jan Beulich wrote:
> Whether the MFN changes does not depend on the new entry being valid
> (but solely on the old one), and the need to update or TLB-flush also
> depends on permission changes.
>
> Signed-off-by: Jan Beulich 
> ---
> v2: Split from larger patch. Fix logic determining whether to update/
> flush IOMMU mappings.
> ---
> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>  is an apparent inconsistency with PoD handling: 2M mappings get
>  valid entries created, while 4k mappings don't. It would seem to
>  me that the 4k case needs changing, even if today this may only
>  be a latent bug. Question of course is why we don't rely on
>  p2m_type_to_flags() doing its job properly and instead special
>  case certain P2M types.
> TBD: Rip out hap-pt-share code from the file altogether?
>
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -494,7 +494,18 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>  l3_pgentry_t l3e_content;
>  int rc;
>  unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
> -unsigned long old_mfn = 0;
> +/*
> + * old_mfn and iommu_old_flags control possible flush/update needs on the
> + * IOMMU: We need to flush when MFN or flags (i.e. permissions) change.
> + * iommu_old_flags being initialized to zero covers the case of the entry
> + * getting replaced being a non-present (leaf or intermediate) one. For
> + * present leaf entries the real value will get calculated below, while
> + * for present intermediate entries ~0 (guaranteed != iommu_pte_flags)
> + * will be used (to cover all cases of what the leaf entries underneath
> + * the intermediate one might be).
> + */
> +unsigned int flags, iommu_old_flags = 0;
> +unsigned long old_mfn = INVALID_MFN;
>  
>  ASSERT(sve != 0);
>  
> @@ -543,12 +554,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> L3_PAGETABLE_SHIFT - PAGE_SHIFT,
> L3_PAGETABLE_ENTRIES);
>  ASSERT(p2m_entry);
> -if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> - !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> +flags = l1e_get_flags(*p2m_entry);
> +if ( flags & _PAGE_PRESENT )
>  {
> -/* We're replacing a non-SP page with a superpage.  Make sure to
> - * handle freeing the table properly. */
> -intermediate_entry = *p2m_entry;
> +if ( flags & _PAGE_PSE )
> +{
> +iommu_old_flags =
> +p2m_get_iommu_flags(p2m_flags_to_type(flags));
> +old_mfn = l1e_get_pfn(*p2m_entry);
> +}
> +else
> +{
> +iommu_old_flags = ~0;
> +intermediate_entry = *p2m_entry;
> +}
>  }
>  
>  ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -559,10 +578,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>  entry_content.l1 = l3e_content.l3;
>  
>  if ( entry_content.l1 != 0 )
> -{
>  p2m_add_iommu_flags(_content, 0, iommu_pte_flags);
> -old_mfn = l1e_get_pfn(*p2m_entry);
> -}
>  
>  p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
>  /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> @@ -587,7 +603,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>  p2m_entry = p2m_find_entry(table, _remainder, gfn,
> 0, L1_PAGETABLE_ENTRIES);
>  ASSERT(p2m_entry);
> -
> +iommu_old_flags =
> +
> p2m_get_iommu_flags(p2m_flags_to_type(l1e_get_flags(*p2m_entry)));
> +old_mfn = l1e_get_pfn(*p2m_entry);
> +
>  if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct)
>  || p2m_is_paging(p2mt) )
>  entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
> @@ -596,10 +615,8 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>  entry_content = l1e_empty();
>  
>  if ( entry_content.l1 != 0 )
> -{
>  p2m_add_iommu_flags(_content, 0, iommu_pte_flags);
> -old_mfn = l1e_get_pfn(*p2m_entry);
> -}
> +
>  /* level 1 entry */
>  p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
>  /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> @@ -610,14 +627,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> L2_PAGETABLE_SHIFT - PAGE_SHIFT,
> L2_PAGETABLE_ENTRIES);
>  ASSERT(p2m_entry);
> -
> -/* FIXME: Deal with 4k replaced by 2meg pages */
> -if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> - !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> -{
> -/* We're replacing a non-SP page with a 

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Wei Liu]

On Thu, Oct 01, 2015 at 04:25:01AM -0600, Jan Beulich wrote:
> Whether the MFN changes does not depend on the new entry being valid
> (but solely on the old one), and the need to update or TLB-flush also
> depends on permission changes.
> 
> Signed-off-by: Jan Beulich 

Release-acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[Andrew Cooper]

On 01/10/15 15:36, Jan Beulich wrote:
 On 01.10.15 at 15:34,  wrote:
>> On 01/10/15 11:25, Jan Beulich wrote:
>>> @@ -645,11 +665,12 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
>>>   && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
>>>  p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
>>>  
>>> -if ( iommu_enabled && need_iommu(p2m->domain) )
>>> +if ( iommu_enabled && need_iommu(p2m->domain) &&
>>> + (iommu_old_flags != iommu_pte_flags || old_mfn != mfn_x(mfn)) )
>>>  {
>>>  if ( iommu_use_hap_pt(p2m->domain) )
>>>  {
>>> -if ( old_mfn && (old_mfn != mfn_x(mfn)) )
>>> +if ( iommu_old_flags )
>>>  amd_iommu_flush_pages(p2m->domain, gfn, page_order);
>> iommu_hap_pt_share is hardwired to 0 on AMD, making this if() clause
>> effectively dead.  Is this what you mean by your second TBD?  I would
>> suggest dropping it.
> Yes, that's what I mean.
>
>>>  }
>>>  else
>>>
>>>
>> In this else clause there is a now-shadowed "flags" which might better
>> be renamed to iommu_flags to avoid confusion.
>>
>> There is also an extra shadowed 'i' which could do with removing, as it
>> introduces a 64bit->32bit truncation (which is not currently a problem).
> See the (not re-submitted because it didn't really change) follow-up
> patch
> (http://lists.xenproject.org/archives/html/xen-devel/2015-09/msg02585.html)
> which deletes that pointless variable altogether.

Ah - I appear to have missed that in my review queue - apologies. 
Consider it Reviewed-by: Andrew Cooper 

As for the logic presented in this patch, I believe it is correct, so
also Reviewed-by: Andrew Cooper 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/p2m-pt: tighten conditions of IOMMU mapping updates

[George Dunlap]

On 01/10/15 11:25, Jan Beulich wrote:
> Whether the MFN changes does not depend on the new entry being valid
> (but solely on the old one), and the need to update or TLB-flush also
> depends on permission changes.
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: George Dunlap 

> ---
> v2: Split from larger patch. Fix logic determining whether to update/
> flush IOMMU mappings.
> ---
> TBD: As already mentioned on the large-page-MMIO-mapping patch, there
>  is an apparent inconsistency with PoD handling: 2M mappings get
>  valid entries created, while 4k mappings don't. It would seem to
>  me that the 4k case needs changing, even if today this may only
>  be a latent bug. Question of course is why we don't rely on
>  p2m_type_to_flags() doing its job properly and instead special
>  case certain P2M types.

The inconsistency in the conditionals there is a bit strange; but I'm
pretty sure that in the 2MB case it is (at the moment) superfluous,
because at the moment it seems that when setting a page with type
p2m_populate_on_demand, it's always passing in _mfn(0), which is valid.

(It used to pass a magic MFN, but Tim Deegan switched it to _mfn(0) at
some point without comment.)


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel