Thes macros really ought to live in the common xen/iommu.h header rather
then being distributed amongst architecture specific iommu headers and
xen/sched.h. This patch moves them there.
NOTE: Disabling 'sharept' in the command line iommu options should really
be hard error on ARM (as opposed to just being ignored), so avoid
parsing that option if CONFIG_ARM is set.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wl@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
xen/drivers/passthrough/iommu.c | 2 ++
xen/include/asm-arm/iommu.h | 3 ---
xen/include/asm-x86/iommu.h | 4 ----
xen/include/xen/iommu.h | 7 +++++++
xen/include/xen/sched.h | 6 ------
5 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 30976b4406..67855eeed5 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -102,8 +102,10 @@ static int __init parse_iommu_param(const char *s)
iommu_hwdom_passthrough = val;
else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 )
iommu_hwdom_strict = val;
+#ifndef CONFIG_ARM
else if ( (val = parse_boolean("sharept", s, ss)) >= 0 )
iommu_hap_pt_share = val;
+#endif
else
rc = -EINVAL;
diff --git a/xen/include/asm-arm/iommu.h b/xen/include/asm-arm/iommu.h
index e5050636d7..77a94b29eb 100644
--- a/xen/include/asm-arm/iommu.h
+++ b/xen/include/asm-arm/iommu.h
@@ -20,9 +20,6 @@ struct arch_iommu
void *priv;
};
-/* Always share P2M Table between the CPU and the IOMMU */
-#define iommu_use_hap_pt(d) (is_iommu_enabled(d))
-
const struct iommu_ops *iommu_get_ops(void);
void iommu_set_ops(const struct iommu_ops *ops);
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index 6d024d5c0e..25d2aee9a9 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -79,10 +79,6 @@ struct iommu_init_ops {
extern const struct iommu_init_ops *iommu_init_ops;
-/* 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)
-
void iommu_update_ire_from_apic(unsigned int apic, unsigned int reg, unsigned int value);
unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg);
int iommu_setup_hpet_msi(struct msi_desc *);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 4b6871936c..45ec6cfe44 100644
--- 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 the IOMMU pagetable need to be kept synchronized with the P2M */
+#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
+
int __must_check iommu_suspend(void);
void iommu_resume(void);
void iommu_crash_shutdown(void);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 538be7120b..6568f2b85b 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -963,12 +963,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
cpumask_weight(v->cpu_hard_affinity) == 1);
}
-#ifdef CONFIG_HAS_PASSTHROUGH
-#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
-#else
-#define need_iommu_pt_sync(d) false
-#endif
-
static inline bool is_vcpu_online(const struct vcpu *v)
{
return !test_bit(_VPF_down, &v->pause_flags);
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develOn 30.07.2019 15:44, Paul Durrant wrote:
> Thes macros really ought to live in the common xen/iommu.h header rather
> then being distributed amongst architecture specific iommu headers and
> xen/sched.h. This patch moves them there.
>
> NOTE: Disabling 'sharept' in the command line iommu options should really
> be hard error on ARM (as opposed to just being ignored), so avoid
> parsing that option if CONFIG_ARM is set.
Agreed. At that point the latest it would perhaps be good to have
Arm have
#define iommu_hap_pt_share true
> --- 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?
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -963,12 +963,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
> cpumask_weight(v->cpu_hard_affinity) == 1);
> }
>
> -#ifdef CONFIG_HAS_PASSTHROUGH
> -#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> -#else
> -#define need_iommu_pt_sync(d) false
> -#endif
The "#else" part of this gets lost - is this intentional, i.e.
are there no references left that could be a problem without
HAS_PASSTHROUGH?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 07 August 2019 11:41
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; 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
>
> On 30.07.2019 15:44, Paul Durrant wrote:
> > Thes macros really ought to live in the common xen/iommu.h header rather
> > then being distributed amongst architecture specific iommu headers and
> > xen/sched.h. This patch moves them there.
> >
> > NOTE: Disabling 'sharept' in the command line iommu options should really
> > be hard error on ARM (as opposed to just being ignored), so avoid
> > parsing that option if CONFIG_ARM is set.
>
> Agreed. At that point the latest it would perhaps be good to have
> Arm have
> #define iommu_hap_pt_share true
I don't quite follow. iommu_hap_pt_share is a global bool_t defined in passthrough/iommu.c... I'm just preventing an ARM command line from being able to change the value... so in effect it will always be true for ARM.
>
> > --- 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.
>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -963,12 +963,6 @@ static inline bool is_hwdom_pinned_vcpu(const struct vcpu *v)
> > cpumask_weight(v->cpu_hard_affinity) == 1);
> > }
> >
> > -#ifdef CONFIG_HAS_PASSTHROUGH
> > -#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
> > -#else
> > -#define need_iommu_pt_sync(d) false
> > -#endif
>
> The "#else" part of this gets lost - is this intentional, i.e.
> are there no references left that could be a problem without
> HAS_PASSTHROUGH?
Not that it can be turned off at the moment, but yes there does appear to be a problem with gnttab_need_iommu_mapping() if I force HAS_PASSTHROUGH off... I'll add an equivalent ifdef.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 14.08.2019 12:13, Paul Durrant wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 07 August 2019 11:41 >> >> On 30.07.2019 15:44, Paul Durrant wrote: >>> Thes macros really ought to live in the common xen/iommu.h header rather >>> then being distributed amongst architecture specific iommu headers and >>> xen/sched.h. This patch moves them there. >>> >>> NOTE: Disabling 'sharept' in the command line iommu options should really >>> be hard error on ARM (as opposed to just being ignored), so avoid >>> parsing that option if CONFIG_ARM is set. >> >> Agreed. At that point the latest it would perhaps be good to have >> Arm have >> #define iommu_hap_pt_share true > > I don't quite follow. iommu_hap_pt_share is a global bool_t defined in passthrough/iommu.c... I'm just preventing an ARM command line from being able to change the value... so in effect it will always be true for ARM. The point of my comment was to allow the compiler to eliminate dead (on Arm) code. Along the lines of Julien's subsequent replies, things like hap_enabled() and the one here should imo be compile time constants (rather than runtime ones) for this reason. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
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?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
> -----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.
Paul
> Cheers,
>
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
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).
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?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
> -----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
Hi,
On 14/08/2019 12:11, Paul Durrant wrote:
>> -----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.
Any CONFIG_{ARM, X86} feels an abuse of common code. So I am always in favor of
dropping them :). My concern is that a few of the helpers will likely return a
single value for any architecture by x86. But that's a different problem...
>
>>
>> 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?
Looking at the patch #6, iommu_use_hap_pt() is reimplemented with:
#define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share)
You also have a comment mentioning Arm systems in the same patch.
So I would be happy with this patch assuming that patch #6 does not change.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
> -----Original Message-----
[snip]
> >>>>>>> +/* 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.
>
> Any CONFIG_{ARM, X86} feels an abuse of common code. So I am always in favor of
> dropping them :). My concern is that a few of the helpers will likely return a
> single value for any architecture by x86. But that's a different problem...
>
> >
> >>
> >> 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?
>
> Looking at the patch #6, iommu_use_hap_pt() is reimplemented with:
>
> #define iommu_use_hap_pt(d) (dom_iommu(d)->hap_pt_share)
>
> You also have a comment mentioning Arm systems in the same patch.
>
> So I would be happy with this patch assuming that patch #6 does not change.
>
Ok, thanks. I'll see about adding a patch for arch specific defs of things like is_hvm_domain() and is_pv_domain(), and hap_enabled().
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2026 Red Hat, Inc.