| Message ID | 1402935486-29136-12-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) > if ( !iommu_enabled || !hd->platform_ops ) > return; > > + arch_iommu_domain_destroy(d); > + > if ( need_iommu(d) ) > iommu_teardown(d); > - > - arch_iommu_domain_destroy(d); At the first glance this doesn't look right, including the explanation you gave (why would devices still be assigned to a guest at this point). And it's rather hard to properly decide with the series here depending on two other series, i.e. there not being a arch_iommu_domain_destroy() at all in current staging. Jan
Hi Ian, On 17/06/14 09:07, Jan Beulich wrote: >>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >> if ( !iommu_enabled || !hd->platform_ops ) >> return; >> >> + arch_iommu_domain_destroy(d); >> + >> if ( need_iommu(d) ) >> iommu_teardown(d); >> - >> - arch_iommu_domain_destroy(d); > > At the first glance this doesn't look right, including the explanation > you gave (why would devices still be assigned to a guest at this > point). Because the toolstack may forget to deassign a device. How do you handle this case in x86? In the SMMU case, this will mean a memory leak and misconfiguration of the registers. I think it's safer to let Xen deassign the remaining devices. > And it's rather hard to properly decide with the series here > depending on two other series, i.e. there not being a > arch_iommu_domain_destroy() at all in current staging. Are you sure? The other series doesn't deal with the IOMMU stuff. This change has been pushed upstream a month ago (see commit 4905b35c " iommu: introduce arch specific code"). Regards,
>>> On 17.06.14 at 11:18, <julien.grall@linaro.org> wrote: > Hi Ian, > > On 17/06/14 09:07, Jan Beulich wrote: >>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>> --- a/xen/drivers/passthrough/iommu.c >>> +++ b/xen/drivers/passthrough/iommu.c >>> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >>> if ( !iommu_enabled || !hd->platform_ops ) >>> return; >>> >>> + arch_iommu_domain_destroy(d); >>> + >>> if ( need_iommu(d) ) >>> iommu_teardown(d); >>> - >>> - arch_iommu_domain_destroy(d); >> >> At the first glance this doesn't look right, including the explanation >> you gave (why would devices still be assigned to a guest at this >> point). > > Because the toolstack may forget to deassign a device. How do you handle > this case in x86? In the SMMU case, this will mean a memory leak and > misconfiguration of the registers. Proper tool stack behavior is required (and not just here). >> And it's rather hard to properly decide with the series here >> depending on two other series, i.e. there not being a >> arch_iommu_domain_destroy() at all in current staging. > > Are you sure? The other series doesn't deal with the IOMMU stuff. This > change has been pushed upstream a month ago (see commit 4905b35c " > iommu: introduce arch specific code"). Oops, indeed - I'm sorry, I looked at a stale branch. Looking at the correct code I still think the current order is the correct one, and if you need to take extra steps you ought to do so from the .teardown hook. Jan
On 06/17/2014 10:29 AM, Jan Beulich wrote: >>>> On 17.06.14 at 11:18, <julien.grall@linaro.org> wrote: >> On 17/06/14 09:07, Jan Beulich wrote: >>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>> --- a/xen/drivers/passthrough/iommu.c >>>> +++ b/xen/drivers/passthrough/iommu.c >>>> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >>>> if ( !iommu_enabled || !hd->platform_ops ) >>>> return; >>>> >>>> + arch_iommu_domain_destroy(d); >>>> + >>>> if ( need_iommu(d) ) >>>> iommu_teardown(d); >>>> - >>>> - arch_iommu_domain_destroy(d); >>> >>> At the first glance this doesn't look right, including the explanation >>> you gave (why would devices still be assigned to a guest at this >>> point). >> >> Because the toolstack may forget to deassign a device. How do you handle >> this case in x86? In the SMMU case, this will mean a memory leak and >> misconfiguration of the registers. > > Proper tool stack behavior is required (and not just here). I think this is important to handle toolstack failure (such as crash) just in case. Hence it doesn't add much code for this purpose. >>> And it's rather hard to properly decide with the series here >>> depending on two other series, i.e. there not being a >>> arch_iommu_domain_destroy() at all in current staging. >> >> Are you sure? The other series doesn't deal with the IOMMU stuff. This >> change has been pushed upstream a month ago (see commit 4905b35c " >> iommu: introduce arch specific code"). > > Oops, indeed - I'm sorry, I looked at a stale branch. Looking at the > correct code I still think the current order is the correct one, and if > you need to take extra steps you ought to do so from the .teardown > hook. I though about implement it in .teardown, but it results to non-obvious code. I could call iommu_dt_domain_destroy in .teardown, that will mean to call "arch dt" code in the SMMU drivers which I think break the design. I would prefer call it the arch specific function. Do you mind if I add a new function called arch_iommu_reassign_devices? This function will reassign every devices of a given domain to the hardware domain. The iommmu_domain_destroy will look like: void iommu_domain_destroy(struct domain *d) { if ( !iommu_enabled ) return; arch_iommu_reassign_devices(d); if ( need_iommu(d) ) iommu_teardown(d); arch_iommu_domain_destroy(d); } Regards,
>>> On 17.06.14 at 14:38, <julien.grall@linaro.org> wrote: > On 06/17/2014 10:29 AM, Jan Beulich wrote: >>>>> On 17.06.14 at 11:18, <julien.grall@linaro.org> wrote: >>> On 17/06/14 09:07, Jan Beulich wrote: >>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>> --- a/xen/drivers/passthrough/iommu.c >>>>> +++ b/xen/drivers/passthrough/iommu.c >>>>> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >>>>> if ( !iommu_enabled || !hd->platform_ops ) >>>>> return; >>>>> >>>>> + arch_iommu_domain_destroy(d); >>>>> + >>>>> if ( need_iommu(d) ) >>>>> iommu_teardown(d); >>>>> - >>>>> - arch_iommu_domain_destroy(d); >>>> >>>> At the first glance this doesn't look right, including the explanation >>>> you gave (why would devices still be assigned to a guest at this >>>> point). >>> >>> Because the toolstack may forget to deassign a device. How do you handle >>> this case in x86? In the SMMU case, this will mean a memory leak and >>> misconfiguration of the registers. >> >> Proper tool stack behavior is required (and not just here). > > I think this is important to handle toolstack failure (such as crash) > just in case. Hence it doesn't add much code for this purpose. If you think this is necessary, then there's no reason to make this ARM-specific (which in turn would eliminate the need for this to sit in an arch hook). Jan
On 06/17/2014 02:04 PM, Jan Beulich wrote: >>>> On 17.06.14 at 14:38, <julien.grall@linaro.org> wrote: >> On 06/17/2014 10:29 AM, Jan Beulich wrote: >>>>>> On 17.06.14 at 11:18, <julien.grall@linaro.org> wrote: >>>> On 17/06/14 09:07, Jan Beulich wrote: >>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>> --- a/xen/drivers/passthrough/iommu.c >>>>>> +++ b/xen/drivers/passthrough/iommu.c >>>>>> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >>>>>> if ( !iommu_enabled || !hd->platform_ops ) >>>>>> return; >>>>>> >>>>>> + arch_iommu_domain_destroy(d); >>>>>> + >>>>>> if ( need_iommu(d) ) >>>>>> iommu_teardown(d); >>>>>> - >>>>>> - arch_iommu_domain_destroy(d); >>>>> >>>>> At the first glance this doesn't look right, including the explanation >>>>> you gave (why would devices still be assigned to a guest at this >>>>> point). >>>> >>>> Because the toolstack may forget to deassign a device. How do you handle >>>> this case in x86? In the SMMU case, this will mean a memory leak and >>>> misconfiguration of the registers. >>> >>> Proper tool stack behavior is required (and not just here). >> >> I think this is important to handle toolstack failure (such as crash) >> just in case. Hence it doesn't add much code for this purpose. > > If you think this is necessary, then there's no reason to make this > ARM-specific (which in turn would eliminate the need for this to sit > in an arch hook). We will have to do DT/PCI specific as I don't use the same way to know which device is assigned to which guest. I won't be able to write (or at least test) the piece of code for the PCI part as my ARM board doesn't support PCI and I don't have a Xen setup for x86. Regards,
>>> On 18.06.14 at 14:24, <julien.grall@linaro.org> wrote: > On 06/17/2014 02:04 PM, Jan Beulich wrote: >>>>> On 17.06.14 at 14:38, <julien.grall@linaro.org> wrote: >>> On 06/17/2014 10:29 AM, Jan Beulich wrote: >>>>>>> On 17.06.14 at 11:18, <julien.grall@linaro.org> wrote: >>>>> On 17/06/14 09:07, Jan Beulich wrote: >>>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote: >>>>>>> --- a/xen/drivers/passthrough/iommu.c >>>>>>> +++ b/xen/drivers/passthrough/iommu.c >>>>>>> @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) >>>>>>> if ( !iommu_enabled || !hd->platform_ops ) >>>>>>> return; >>>>>>> >>>>>>> + arch_iommu_domain_destroy(d); >>>>>>> + >>>>>>> if ( need_iommu(d) ) >>>>>>> iommu_teardown(d); >>>>>>> - >>>>>>> - arch_iommu_domain_destroy(d); >>>>>> >>>>>> At the first glance this doesn't look right, including the explanation >>>>>> you gave (why would devices still be assigned to a guest at this >>>>>> point). >>>>> >>>>> Because the toolstack may forget to deassign a device. How do you handle >>>>> this case in x86? In the SMMU case, this will mean a memory leak and >>>>> misconfiguration of the registers. >>>> >>>> Proper tool stack behavior is required (and not just here). >>> >>> I think this is important to handle toolstack failure (such as crash) >>> just in case. Hence it doesn't add much code for this purpose. >> >> If you think this is necessary, then there's no reason to make this >> ARM-specific (which in turn would eliminate the need for this to sit >> in an arch hook). > > We will have to do DT/PCI specific as I don't use the same way to know > which device is assigned to which guest. Which sounds wrong from a design pov. But again, it's the tool stack maintainers' call in the end. Jan
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 2e9b48d..d71ab03 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -219,10 +219,10 @@ void iommu_domain_destroy(struct domain *d) if ( !iommu_enabled || !hd->platform_ops ) return; + arch_iommu_domain_destroy(d); + if ( need_iommu(d) ) iommu_teardown(d); - - arch_iommu_domain_destroy(d); } int iommu_map_page(struct domain *d, unsigned long gfn, unsigned long mfn,
arch_iommu_domain_destroy contains specific architecture code. On x86, this code will clean up the ioport_list which is not used in both iommu (i.e AMD & x86) drivers. On ARM, the toolstack may not have deassign every device to the guest. Therefore, we have to go through the device list and removing them before asking the IOMMU drivers to release memory for this domain. This is done by iommu_dt_domain_destroy which is called by arch_iommu_domain_destroy. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Jan Beulich <jbeulich@suse.com> --- xen/drivers/passthrough/iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)