Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Xu, Quan]

On May 24, 2016 4:34 PM, Jan Beulich  wrote:
> but indeed if you
> drop the annotations from non-IOMMU functions (unless, as said, you mean
> to also add them further up the call trees), then I think things should be
> coming close.
> 

I'll drop the annotations from non-IOMMU functions.
Quan

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

Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Jan Beulich]

>>> On 24.05.16 at 10:11,  wrote:
> I thought the IOMMU / MM boundary is the MM functions (high level callers) 
> which call iommu_* interfaces (such as,  iommu_map_page / iommu_unmap_page / 
> iommu_iotlb_flush ...).

Exactly - the boundary is _in_ those MM functions, at the points where
they call IOMMU ones.

> For this case, the xenmem_add_to_physmap() indeed calls iommu_iotlb_flush(), 
>  but xenmem_add_to_physmap() may be hypervisor interface, instead of MM 
> interface.
> 
> If I drop this __must_check and fix patch 3 / patch 5, then I think 
> __must_check would not be a block issue.

Not sure what "a block issue" is supposed to mean here, but indeed
if you drop the annotations from non-IOMMU functions (unless, as
said, you mean to also add them further up the call trees), then I
think things should be coming close.

Jan


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

Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Xu, Quan]

On May 24, 2016 3:09 PM, Tian, Kevin  wrote:
> > From: Jan Beulich [mailto:jbeul...@suse.com]
> > Sent: Tuesday, May 24, 2016 3:02 PM
> >
> > >>> On 24.05.16 at 03:16,  wrote:
> > > On May 24, 2016 12:06 AM, Jan Beulich  wrote:
> > >> >>> On 18.05.16 at 10:08,  wrote:
> > >> > --- a/xen/common/memory.c
> > >> > +++ b/xen/common/memory.c
> > >> > @@ -633,9 +633,9 @@ static long
> > >>
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t)
> > >> arg)
> > >> >  return rc;
> > >> >  }
> > >> >
> > >> > -static int xenmem_add_to_physmap(struct domain *d,
> > >> > - struct xen_add_to_physmap *xatp,
> > >> > - unsigned int start)
> > >> > +static int __must_check xenmem_add_to_physmap(struct domain *d,
> > >> > +  struct
> > >> > +xen_add_to_physmap
> > > *xatp,
> > >> > +  unsigned int
> > >> > + start)
> > >> >  {
> > >>
> > >> As before - either you do this adding of annotations completely, or
> > >> you stop
> > > at
> > >> the IOMMU / MM boundary.
> > >
> > > I prefer to stop at the IOMMU / MM boundary. The IOMMU boundary is
> > > obvious, but what's the definition of MM boundary? I thought this is at
> MM boundary.
> >
> > Not sure what you mean to understand. The IOMMU / MM boundary is the
> > boundary between those two components, there's no talk of two
> > boundaries here, and hence the question is unclear to me.
> >
> > Jan
> 
> Hi, Quan,
> 
> A file-based map about IOMMU/MM boundary is under arch/x86/mm. You
> need focus on low-level interaction between IOMMU and MM components,
> i.e. when some state change in MM code (mostly p2m change) needs to
> conduct IOMMU operations.
> 
> Above xenmem is much higher level, which will be routed to various MM
> operations internally so you don't need bother here.

Jan / Kevin,

I thought the IOMMU / MM boundary is the MM functions (high level callers) 
which call iommu_* interfaces (such as,  iommu_map_page / iommu_unmap_page / 
iommu_iotlb_flush ...).
For this case, the xenmem_add_to_physmap() indeed calls iommu_iotlb_flush(),  
but xenmem_add_to_physmap() may be hypervisor interface, instead of MM 
interface.

If I drop this __must_check and fix patch 3 / patch 5, then I think 
__must_check would not be a block issue.

Quan

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

Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Tian, Kevin]

> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Tuesday, May 24, 2016 3:02 PM
> 
> >>> On 24.05.16 at 03:16,  wrote:
> > On May 24, 2016 12:06 AM, Jan Beulich  wrote:
> >> >>> On 18.05.16 at 10:08,  wrote:
> >> > --- a/xen/common/memory.c
> >> > +++ b/xen/common/memory.c
> >> > @@ -633,9 +633,9 @@ static long
> >> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t)
> >> arg)
> >> >  return rc;
> >> >  }
> >> >
> >> > -static int xenmem_add_to_physmap(struct domain *d,
> >> > - struct xen_add_to_physmap *xatp,
> >> > - unsigned int start)
> >> > +static int __must_check xenmem_add_to_physmap(struct domain *d,
> >> > +  struct xen_add_to_physmap
> > *xatp,
> >> > +  unsigned int start)
> >> >  {
> >>
> >> As before - either you do this adding of annotations completely, or you 
> >> stop
> > at
> >> the IOMMU / MM boundary.
> >
> > I prefer to stop at the IOMMU / MM boundary. The IOMMU boundary is obvious,
> > but what's the definition of MM boundary? I thought this is at MM boundary.
> 
> Not sure what you mean to understand. The IOMMU / MM boundary
> is the boundary between those two components, there's no talk of
> two boundaries here, and hence the question is unclear to me.
> 
> Jan

Hi, Quan,

A file-based map about IOMMU/MM boundary is under arch/x86/mm. You
need focus on low-level interaction between IOMMU and MM components,
i.e. when some state change in MM code (mostly p2m change) needs to 
conduct IOMMU operations.

Above xenmem is much higher level, which will be routed to various MM 
operations internally so you don't need bother here.

Thanks
Kevin

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

Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Jan Beulich]

>>> On 24.05.16 at 03:16,  wrote:
> On May 24, 2016 12:06 AM, Jan Beulich  wrote:
>> >>> On 18.05.16 at 10:08,  wrote:
>> > --- a/xen/common/memory.c
>> > +++ b/xen/common/memory.c
>> > @@ -633,9 +633,9 @@ static long
>> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t)
>> arg)
>> >  return rc;
>> >  }
>> >
>> > -static int xenmem_add_to_physmap(struct domain *d,
>> > - struct xen_add_to_physmap *xatp,
>> > - unsigned int start)
>> > +static int __must_check xenmem_add_to_physmap(struct domain *d,
>> > +  struct xen_add_to_physmap 
> *xatp,
>> > +  unsigned int start)
>> >  {
>> 
>> As before - either you do this adding of annotations completely, or you stop 
> at
>> the IOMMU / MM boundary. 
> 
> I prefer to stop at the IOMMU / MM boundary. The IOMMU boundary is obvious, 
> but what's the definition of MM boundary? I thought this is at MM boundary.

Not sure what you mean to understand. The IOMMU / MM boundary
is the boundary between those two components, there's no talk of
two boundaries here, and hence the question is unclear to me.

Jan


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

Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Xu, Quan]

On May 24, 2016 12:06 AM, Jan Beulich  wrote:
> >>> On 18.05.16 at 10:08,  wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -633,9 +633,9 @@ static long
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t)
> arg)
> >  return rc;
> >  }
> >
> > -static int xenmem_add_to_physmap(struct domain *d,
> > - struct xen_add_to_physmap *xatp,
> > - unsigned int start)
> > +static int __must_check xenmem_add_to_physmap(struct domain *d,
> > +  struct xen_add_to_physmap 
> > *xatp,
> > +  unsigned int start)
> >  {
> 
> As before - either you do this adding of annotations completely, or you stop 
> at
> the IOMMU / MM boundary. 

I prefer to stop at the IOMMU / MM boundary. The IOMMU boundary is obvious, but 
what's the definition of MM boundary? I thought this is at MM boundary.

Quan

> And note that this addition was not covered by
> the R-b tag I had offered during the
> v4 review.


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

Re: [Xen-devel] [PATCH v5 06/10] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones).

[Jan Beulich]

>>> On 18.05.16 at 10:08,  wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -633,9 +633,9 @@ static long 
> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  return rc;
>  }
>  
> -static int xenmem_add_to_physmap(struct domain *d,
> - struct xen_add_to_physmap *xatp,
> - unsigned int start)
> +static int __must_check xenmem_add_to_physmap(struct domain *d,
> +  struct xen_add_to_physmap 
> *xatp,
> +  unsigned int start)
>  {

As before - either you do this adding of annotations completely, or
you stop at the IOMMU / MM boundary. And note that this
addition was not covered by the R-b tag I had offered during the
v4 review.

Jan


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