Re: [Xen-devel] [PATCH v8 6/6] introduce a 'passthrough' configuration option to xl.cfg...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 05 September 2019 20:59
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Jan Beulich <jbeulich@xxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; 
> Wei Liu <wl@xxxxxxx>;
> Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>; Konrad Rzeszutek
> Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> Tim (Xen.org)
> <tim@xxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; Christian Lindig
> <christian.lindig@xxxxxxxxxx>; David Scott <dave@xxxxxxxxxx>; Volodymyr 
> Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v8 6/6] introduce a 'passthrough' configuration option to 
> xl.cfg...
> 
> Hi,
> 
> On 9/2/19 3:50 PM, Paul Durrant wrote:
> > @@ -263,9 +263,17 @@ struct domain_iommu {
> >       DECLARE_BITMAP(features, IOMMU_FEAT_count);
> >
> >       /*
> > -     * Does the guest reqire mappings to be synchonized, to maintain
> > -     * the default dfn == pfn map. (See comment on dfn at the top of
> > -     * include/xen/mm.h).
> > +     * Does the guest share HAP mapping with the IOMMU? This is always
> > +     * true for ARM systems and may be true for x86 systems where the
> > +     * the hardware is capable.
> > +     */
> 
> I am worried that such comment may rot over the time. For instance, if
> we either add a new architecture or decide to stop sharing PT on Arm.
> 
> I vaguely recall some potential issues with the MSI doorbells that would
> require us to unshare the PT when they will be supported in guest.
> 
> I would suggest to either refers to the implementation of
> iommu_use_hap_pt() or drop completely the second sentence.

Ok, I'll just drop the sentence.

  Paul

> 
> > +    bool hap_pt_share;
> > +
> > +    /*
> > +     * Does the guest require mappings to be synchronized, to maintain
> > +     * the default dfn == pfn map? (See comment on dfn at the top of
> > +     * include/xen/mm.h). Note that hap_pt_share == false does not
> > +     * necessarily imply this is true.
> >        */
> >       bool need_sync;
> >   };
> > @@ -275,8 +283,7 @@ struct domain_iommu {
> >   #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)
> > +#define iommu_use_hap_pt(d)       (dom_iommu(d)->hap_pt_share)
> >
> >   /* Does the IOMMU pagetable need to be kept synchronized with the P2M */
> >   #ifdef CONFIG_HAS_PASSTHROUGH
> >
> 
> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel