Re: [Xen-devel] [PATCH 2/6] use is_iommu_enabled() where appropriate...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 07 August 2019 10:56
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Brian Woods <brian.woods@xxxxxxx>; 
> Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Andrew 
> Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; 
> VolodymyrBabchuk
> <Volodymyr_Babchuk@xxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Jun 
> Nakajima
> <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH 2/6] use is_iommu_enabled() where appropriate...
> 
> On 30.07.2019 15:44, Paul Durrant wrote:
> > --- a/xen/arch/arm/p2m.c
> > +++ b/xen/arch/arm/p2m.c
> > @@ -1531,8 +1531,7 @@ int p2m_init(struct domain *d)
> >        * shared with the CPU, Xen has to make sure that the PT changes have
> >        * reached the memory
> >        */
> > -    p2m->clean_pte = iommu_enabled &&
> > -        !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
> > +    p2m->clean_pte = !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
> 
> I can't tell if the original code was meant to be this way, but I'm
> afraid your transformation is not correct: The prior construct,
> expanding iommu_has_feature(), was
> 
> iommu_enabled && !(iommu_enabled && test_bit(feature, dom_iommu(d)->features))
> 
> which transforms through
> 
> iommu_enabled && (!iommu_enabled || !test_bit(feature, 
> dom_iommu(d)->features))
> 
> to
> 
> (iommu_enabled && !iommu_enabled) || (iommu_enabled && !test_bit(feature, 
> dom_iommu(d)->features))
> 
> and hence
> 
> iommu_enabled && !test_bit(feature, dom_iommu(d)->features)
> 
> whereas the new code simply is
> 
> !(iommu_enabled && test_bit(feature, dom_iommu(d)->features))
> 
> i.e.
> 
> !iommu_enabled || !test_bit(feature, dom_iommu(d)->features)

Yes, somehow I'd read that the iommu_enabled was inverted in the first 
instance. I'll add a check of is_iommu_enabled() back into p2m_init().

> 
> > @@ -766,7 +766,7 @@ ept_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t 
> > mfn,
> >           new_entry.sp = !!i;
> >           new_entry.sa_p2mt = p2mt;
> >           new_entry.access = p2ma;
> > -        new_entry.snp = (iommu_enabled && iommu_snoop);
> > +        new_entry.snp = is_iommu_enabled(p2m->domain) && iommu_snoop;
> 
> Please use d here.
> 
> Seeing that this is the last change in x86/mm/, did you overlook
> the use in p2m_pt_set_entry()? Or is this meant to go on top of
> Alexandru's "x86/mm: Clean IOMMU flags from p2m-pt code" (which
> should then be noted in a post-commit-message remark)?

Yes, it needs to go on top of Alexandru's patch. I said the series is based on 
that patch in the cover letter but I can state it here as well if it helps.

> 
> > @@ -556,7 +556,7 @@ int iommu_do_domctl(
> >   {
> >       int ret = -ENODEV;
> >
> > -    if ( !iommu_enabled )
> > +    if ( !is_iommu_enabled(d) )
> >           return -ENOSYS;
> 
> ENOSYS was wrong here from the beginning, but it certainly gets
> worse with this no longer being a system wide property. Please
> change to EOPNOTSUPP or some other suitable one.

Sure. I'll go with EOPNOTSUPP.

> 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -864,7 +864,7 @@ static int pci_clean_dpci_irqs(struct domain *d)
> 
> Above here there's another use in pci_enable_acs(), which should
> imo act on pdev->domain.

Oh yes, I'll fix that.

> 
> There's another use in flask_iommu_resource_use_perm(). All
> callers of the function hold a struct domain * in their hands,
> which I think they should pass into this function such that the
> conditional can be replaced.

I wasn't sure about this one. It looks more like the perm passed back is based 
on the hardware features available but I guess it will DTRT if the IOMMU is not 
enabled for the domain.

  Paul

> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel