Re: [Xen-devel] [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros

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

> -----Original Message-----
> From: Julien Grall <julien.grall@arm.com>
> Sent: 14 August 2019 11:45
> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>;
> Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
> 
> Hi,
> 
> On 14/08/2019 11:27, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien.grall@arm.com>
> >> Sent: 14 August 2019 11:21
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; 'Jan Beulich' <jbeulich@suse.com>
> >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> >> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>;
> >> Wei Liu <wl@xen.org>
> >> Subject: Re: [PATCH 5/6] iommu: tidy up iommu_us_hap_pt() and need_iommu_pt_sync() macros
> >>
> >> Hi Paul,
> >>
> >> On 14/08/2019 11:13, Paul Durrant wrote:
> >>>>> --- a/xen/include/xen/iommu.h
> >>>>> +++ b/xen/include/xen/iommu.h
> >>>>> @@ -268,6 +268,13 @@ struct domain_iommu {
> >>>>>     #define iommu_set_feature(d, f)   set_bit(f, dom_iommu(d)->features)
> >>>>>     #define iommu_clear_feature(d, f) clear_bit(f, dom_iommu(d)->features)
> >>>>>
> >>>>> +/* Are we using the domain P2M table as its IOMMU pagetable? */
> >>>>> +#define iommu_use_hap_pt(d) \
> >>>>> +    (hap_enabled(d) && is_iommu_enabled(d) && iommu_hap_pt_share)
> >>>>
> >>>> Does this build for Arm, seeing that there's no hap_enabled()
> >>>> definition there? Or have I missed its addition earlier in this
> >>>> series?
> >>>
> >>> It moved to common code sched.h in an earlier patch.
> >>
> >> I went through the series and didn't find where hap_enabled() is defined for Arm
> >> in this series. Do you mind pointing the exact patch?
> >>
> >
> > Sorry, I wasn't clear... The change is in my other series, "use stashed domain create flags", which
> is a pre-requisite for this series (as called out in the cover letter). The change is made in patch #2
> of that series: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02256.html.
> 
> Oh. I understand this adds benefits as the implementation is now common. But the
> downside is hap_enabled() will now require evaluation on Arm even it is
> evaluates to true... This will prevent the compiler to remove any non-HAP code
> paths (assuming there are any in the common code).

There was one in the common iommu code that thus required a #ifdef for ARM.

> 
> Furthermore, 2 parts of the iommu_use_hap_pt() condition will always returning
> always true. But as they are non-constant, so they will always be evaluated.
> 
> It is also probably going to confuse developer as they may think non-HAP is
> supported on Arm. You can't find easily that both hap_enabled(...) and
> iommu_hap_pt_share will always evaluate to true.
> 
> So aside the common implementation, what is the real gain for Arm?

There's no real gain for ARM, the gain is in the reduction in ifdef-ery and thus tidiness of code. I could put back some ifdefs if you'd prefer, or I could just put a comment stating that iommu_use_hap_pt() will always be true for ARM. Which would you prefer?

  Paul

> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel