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: 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