> -----Original Message-----
> From: Julien Grall <julien.gr...@arm.com>
> Sent: 14 August 2019 11:45
> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich' <jbeul...@suse.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monne
> <roger....@citrix.com>; Volodymyr Babchuk <volodymyr_babc...@epam.com>;
> George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Stefano
> Stabellini
> <sstabell...@kernel.org>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>; Tim
> (Xen.org) <t...@xen.org>;
> Wei Liu <w...@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.gr...@arm.com>
> >> Sent: 14 August 2019 11:21
> >> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich'
> >> <jbeul...@suse.com>
> >> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> >> <andrew.coop...@citrix.com>; Roger Pau Monne
> >> <roger....@citrix.com>; Volodymyr Babchuk <volodymyr_babc...@epam.com>;
> >> George Dunlap
> >> <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Stefano
> >> Stabellini
> >> <sstabell...@kernel.org>; Konrad Rzeszutek Wilk <konrad.w...@oracle.com>;
> >> Tim (Xen.org)
> <t...@xen.org>;
> >> Wei Liu <w...@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
