| Message ID | dc85bb73ca4b6ab8b4a2370f2db7700445fbc5f8.1603731279.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Make PCI passthrough code non-x86 specific | expand |
On 26.10.2020 18:17, Rahul Singh wrote: > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -1419,13 +1419,15 @@ 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 > +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING) > + /* 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 -EXDEV; > +#endif Besides this also disabling mem-sharing and log-dirty related logic, I don't think the change is correct: Each item being checked needs individually disabling depending on its associated CONFIG_*. For this, perhaps you want to introduce something like mem_paging_enabled(d), to avoid the need for #ifdef here? Jan
Hello Jan, > On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote: > > On 26.10.2020 18:17, Rahul Singh wrote: >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -1419,13 +1419,15 @@ 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 >> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING) >> + /* 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 -EXDEV; >> +#endif > > Besides this also disabling mem-sharing and log-dirty related > logic, I don't think the change is correct: Each item being > checked needs individually disabling depending on its associated > CONFIG_*. For this, perhaps you want to introduce something > like mem_paging_enabled(d), to avoid the need for #ifdef here? > Ok I will fix that in next version. > Jan Regards, Rahul
Hello Jan, > On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote: > > Hello Jan, > >> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 26.10.2020 18:17, Rahul Singh wrote: >>> --- a/xen/drivers/passthrough/pci.c >>> +++ b/xen/drivers/passthrough/pci.c >>> @@ -1419,13 +1419,15 @@ 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 >>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING) >>> + /* 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 -EXDEV; >>> +#endif >> >> Besides this also disabling mem-sharing and log-dirty related >> logic, I don't think the change is correct: Each item being >> checked needs individually disabling depending on its associated >> CONFIG_*. For this, perhaps you want to introduce something >> like mem_paging_enabled(d), to avoid the need for #ifdef here? >> > > Ok I will fix that in next version. I just check and found out that mem-sharing , men-paging and log-dirty is x86 specific and not implemented for ARM. Is that will be ok if I move above code to x86 specific directory and introduce new function arch_pcidev_is_assignable() that will test if pcidev is assignable or not ? > >> Jan Regards, Rahul
On 29.10.2020 17:58, Rahul Singh wrote: >> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote: >>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote: >>> On 26.10.2020 18:17, Rahul Singh wrote: >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -1419,13 +1419,15 @@ 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 >>>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING) >>>> + /* 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 -EXDEV; >>>> +#endif >>> >>> Besides this also disabling mem-sharing and log-dirty related >>> logic, I don't think the change is correct: Each item being >>> checked needs individually disabling depending on its associated >>> CONFIG_*. For this, perhaps you want to introduce something >>> like mem_paging_enabled(d), to avoid the need for #ifdef here? >>> >> >> Ok I will fix that in next version. > > I just check and found out that mem-sharing , men-paging and log-dirty is x86 specific and not implemented for ARM. > Is that will be ok if I move above code to x86 specific directory and introduce new function arch_pcidev_is_assignable() that will test if pcidev is assignable or not ? As an immediate workaround - perhaps (long term the individual pieces should still be individually checked here, as they're not inherently x86-specific). Since there's no device property involved here, the suggested name looks misleading. Perhaps arch_iommu_usable(d)? Jan
Hello Jan, > On 29 Oct 2020, at 5:16 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 29.10.2020 17:58, Rahul Singh wrote: >>> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote: >>>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 26.10.2020 18:17, Rahul Singh wrote: >>>>> --- a/xen/drivers/passthrough/pci.c >>>>> +++ b/xen/drivers/passthrough/pci.c >>>>> @@ -1419,13 +1419,15 @@ 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 >>>>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING) >>>>> + /* 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 -EXDEV; >>>>> +#endif >>>> >>>> Besides this also disabling mem-sharing and log-dirty related >>>> logic, I don't think the change is correct: Each item being >>>> checked needs individually disabling depending on its associated >>>> CONFIG_*. For this, perhaps you want to introduce something >>>> like mem_paging_enabled(d), to avoid the need for #ifdef here? >>>> >>> >>> Ok I will fix that in next version. >> >> I just check and found out that mem-sharing , men-paging and log-dirty is x86 specific and not implemented for ARM. >> Is that will be ok if I move above code to x86 specific directory and introduce new function arch_pcidev_is_assignable() that will test if pcidev is assignable or not ? > > As an immediate workaround - perhaps (long term the individual > pieces should still be individually checked here, as they're > not inherently x86-specific). Since there's no device property > involved here, the suggested name looks misleading. Perhaps > arch_iommu_usable(d)? Thanks. I will modify the code as per suggestion and will share the version v2 for review. > > Jan Regards, Rahul
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index c6fbb7172c..3125c23e87 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -1419,13 +1419,15 @@ 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 +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING) + /* 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 -EXDEV; +#endif /* device_assigned() should already have cleared the device for assignment */ ASSERT(pcidevs_locked());
d->vm_event_paging struct is defined under CONFIG_HAS_MEM_PAGING in sched.h but referenced in passthrough/pci.c directly. If CONFIG_HAS_MEM_PAGING is not enabled for architecture, compiler will throws an error. No functional change. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- xen/drivers/passthrough/pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)