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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 14 August 2019 11:45
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich' <jbeulich@xxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; 
> George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> 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@xxxxxxx>
> >> Sent: 14 August 2019 11:21
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; 'Jan Beulich' 
> >> <jbeulich@xxxxxxxx>
> >> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper 
> >> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> >> <roger.pau@xxxxxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; 
> >> George Dunlap
> >> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> >> Stabellini
> >> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; 
> >> Tim (Xen.org)
> <tim@xxxxxxx>;
> >> Wei Liu <wl@xxxxxxx>
> >> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel