| Message ID | 7b60501fa689a4f2795ea6c34a7475d288f154a9.1604417224.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Make PCI passthrough code non-x86 specific | expand |
> On 3 Nov 2020, at 15:59, Rahul Singh <Rahul.Singh@arm.com> wrote: > > If mem-sharing, mem-paging and log-dirty functionality is not enabled > for architecture when HAS_PCI is enabled, compiler will throw an error. > > Move code to x86 specific directory to fix compilation error. > > No functional change. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > > Changes in v2: > - Move mem-sharing , men-paging and log-dirty specific code to x86 directory. > > --- > xen/drivers/passthrough/pci.c | 8 +------- > xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++ > xen/include/xen/iommu.h | 2 ++ > 3 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c > index 04d3e2c0f9..433989e654 100644 > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -22,7 +22,6 @@ > #include <xen/iommu.h> > #include <xen/irq.h> > #include <xen/param.h> > -#include <xen/vm_event.h> > #include <xen/delay.h> > #include <xen/keyhandler.h> > #include <xen/event.h> > @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > if ( !is_iommu_enabled(d) ) > return 0; > > - /* Prevent device assign if mem paging or mem sharing have been > - * enabled for this domain */ > - if ( d != dom_io && > - unlikely(mem_sharing_enabled(d) || > - vm_event_check_ring(d->vm_event_paging) || > - p2m_get_hostp2m(d)->global_logdirty) ) > + if( !arch_iommu_usable(d) ) > return -EXDEV; > > /* device_assigned() should already have cleared the device for assignment */ > diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c > index 875e67b53b..b3d151a14c 100644 > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -23,6 +23,7 @@ > #include <asm/hvm/io.h> > #include <asm/io_apic.h> > #include <asm/setup.h> > +#include <xen/vm_event.h> > > const struct iommu_init_ops *__initdata iommu_init_ops; > struct iommu_ops __read_mostly iommu_ops; > @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi( > ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0; > } > > +bool_t arch_iommu_usable(struct domain *d) > +{ > + > + /* Prevent device assign if mem paging or mem sharing have been > + * enabled for this domain */ > + if ( d != dom_io && unlikely(mem_sharing_enabled(d) || > + vm_event_check_ring(d->vm_event_paging) || > + p2m_get_hostp2m(d)->global_logdirty) ) > + return false; > + else > + return true; > +} > /* > * Local variables: > * mode: C > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h > index 191021870f..493528cee3 100644 > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -381,6 +381,8 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb); > extern struct spinlock iommu_pt_cleanup_lock; > extern struct page_list_head iommu_pt_cleanup_list; > > +bool_t arch_iommu_usable(struct domain *d); > + > #endif /* _IOMMU_H_ */ > > /* > -- > 2.17.1 >
On 03.11.2020 16:59, Rahul Singh wrote: > If mem-sharing, mem-paging and log-dirty functionality is not enabled > for architecture when HAS_PCI is enabled, compiler will throw an error. Nit: Is it really "and", not "or"? > @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) > if ( !is_iommu_enabled(d) ) > return 0; > > - /* Prevent device assign if mem paging or mem sharing have been > - * enabled for this domain */ > - if ( d != dom_io && > - unlikely(mem_sharing_enabled(d) || > - vm_event_check_ring(d->vm_event_paging) || > - p2m_get_hostp2m(d)->global_logdirty) ) > + if( !arch_iommu_usable(d) ) > return -EXDEV; While iirc I did suggest this name, seeing it used here leaves me somewhat unhappy with the name, albeit I also can't think of any better alternative right now. Maybe arch_iommu_use_permitted()? > @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi( > ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0; > } > > +bool_t arch_iommu_usable(struct domain *d) Just bool please and I very much hope the parameter can be const. > +{ > + > + /* Prevent device assign if mem paging or mem sharing have been > + * enabled for this domain */ Please correct comment style as you move it. > + if ( d != dom_io && unlikely(mem_sharing_enabled(d) || > + vm_event_check_ring(d->vm_event_paging) || > + p2m_get_hostp2m(d)->global_logdirty) ) You've screwed up indentation, and I don't see why ... > + return false; > + else > + return true; > +} ... this can't be a simple single return statement anyway: return d == dom_io || likely(!mem_sharing_enabled(d) && !vm_event_check_ring(d->vm_event_paging) && !p2m_get_hostp2m(d)->global_logdirty); In the course of moving I'd also suggest dropping the use of likely() here: The way it's used (on an && expression) isn't normally having much effect anyway. If anything it should imo be return d == dom_io || (likely(!mem_sharing_enabled(d)) && likely(!vm_event_check_ring(d->vm_event_paging)) && likely(!p2m_get_hostp2m(d)->global_logdirty)); Any transformation to this effect wants mentioning in the description, though. Jan
Hello Jan, > On 6 Nov 2020, at 9:21 am, Jan Beulich <jbeulich@suse.com> wrote: > > On 03.11.2020 16:59, Rahul Singh wrote: >> If mem-sharing, mem-paging and log-dirty functionality is not enabled >> for architecture when HAS_PCI is enabled, compiler will throw an error. > > Nit: Is it really "and", not "or”? Ok yes I will fix in next version. > >> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) >> if ( !is_iommu_enabled(d) ) >> return 0; >> >> - /* Prevent device assign if mem paging or mem sharing have been >> - * enabled for this domain */ >> - if ( d != dom_io && >> - unlikely(mem_sharing_enabled(d) || >> - vm_event_check_ring(d->vm_event_paging) || >> - p2m_get_hostp2m(d)->global_logdirty) ) >> + if( !arch_iommu_usable(d) ) >> return -EXDEV; > > While iirc I did suggest this name, seeing it used here leaves me > somewhat unhappy with the name, albeit I also can't think of any > better alternative right now. Maybe arch_iommu_use_permitted()? Ok I will modify as per your suggestion. > >> @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi( >> ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0; >> } >> >> +bool_t arch_iommu_usable(struct domain *d) > > Just bool please and I very much hope the parameter can be const. Ok I will fix in next version. > >> +{ >> + >> + /* Prevent device assign if mem paging or mem sharing have been >> + * enabled for this domain */ > > Please correct comment style as you move it. Ok. > >> + if ( d != dom_io && unlikely(mem_sharing_enabled(d) || >> + vm_event_check_ring(d->vm_event_paging) || >> + p2m_get_hostp2m(d)->global_logdirty) ) > > You've screwed up indentation, and I don't see why ... I will fix in next version. > >> + return false; >> + else >> + return true; >> +} > > ... this can't be a simple single return statement anyway: > > return d == dom_io || > likely(!mem_sharing_enabled(d) && > !vm_event_check_ring(d->vm_event_paging) && > !p2m_get_hostp2m(d)->global_logdirty); > > In the course of moving I'd also suggest dropping the use of > likely() here: The way it's used (on an && expression) isn't > normally having much effect anyway. If anything it should imo > be > > return d == dom_io || > (likely(!mem_sharing_enabled(d)) && > likely(!vm_event_check_ring(d->vm_event_paging)) && > likely(!p2m_get_hostp2m(d)->global_logdirty)); > > Any transformation to this effect wants mentioning in the > description, though. Ok I will modify as per your suggestion. > > Jan Regards, Rahul
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index 04d3e2c0f9..433989e654 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -22,7 +22,6 @@ #include <xen/iommu.h> #include <xen/irq.h> #include <xen/param.h> -#include <xen/vm_event.h> #include <xen/delay.h> #include <xen/keyhandler.h> #include <xen/event.h> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag) if ( !is_iommu_enabled(d) ) return 0; - /* Prevent device assign if mem paging or mem sharing have been - * enabled for this domain */ - if ( d != dom_io && - unlikely(mem_sharing_enabled(d) || - vm_event_check_ring(d->vm_event_paging) || - p2m_get_hostp2m(d)->global_logdirty) ) + if( !arch_iommu_usable(d) ) return -EXDEV; /* device_assigned() should already have cleared the device for assignment */ diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 875e67b53b..b3d151a14c 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -23,6 +23,7 @@ #include <asm/hvm/io.h> #include <asm/io_apic.h> #include <asm/setup.h> +#include <xen/vm_event.h> const struct iommu_init_ops *__initdata iommu_init_ops; struct iommu_ops __read_mostly iommu_ops; @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi( ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0; } +bool_t arch_iommu_usable(struct domain *d) +{ + + /* Prevent device assign if mem paging or mem sharing have been + * enabled for this domain */ + if ( d != dom_io && unlikely(mem_sharing_enabled(d) || + vm_event_check_ring(d->vm_event_paging) || + p2m_get_hostp2m(d)->global_logdirty) ) + return false; + else + return true; +} /* * Local variables: * mode: C diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 191021870f..493528cee3 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -381,6 +381,8 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb); extern struct spinlock iommu_pt_cleanup_lock; extern struct page_list_head iommu_pt_cleanup_list; +bool_t arch_iommu_usable(struct domain *d); + #endif /* _IOMMU_H_ */ /*
If mem-sharing, mem-paging and log-dirty functionality is not enabled for architecture when HAS_PCI is enabled, compiler will throw an error. Move code to x86 specific directory to fix compilation error. No functional change. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- Changes in v2: - Move mem-sharing , men-paging and log-dirty specific code to x86 directory. --- xen/drivers/passthrough/pci.c | 8 +------- xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++ xen/include/xen/iommu.h | 2 ++ 3 files changed, 16 insertions(+), 7 deletions(-)