| Message ID | e607a32be8e84c56d65160902f4bd3fb434ee9d3.1649935679.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Retire bus_set_iommu() | expand |
On 2022/4/14 20:42, Robin Murphy wrote: > @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus) > */ > int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) > { > - int err; > - > - if (ops == NULL) { > - bus->iommu_ops = NULL; > - return 0; > - } > - > - if (bus->iommu_ops != NULL) > + if (bus->iommu_ops && ops && bus->iommu_ops != ops) > return -EBUSY; > > bus->iommu_ops = ops; Do we still need to keep above lines in bus_set_iommu()? Best regards, baolu
On 2022-04-16 01:04, Lu Baolu wrote: > On 2022/4/14 20:42, Robin Murphy wrote: >> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus) >> */ >> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) >> { >> - int err; >> - >> - if (ops == NULL) { >> - bus->iommu_ops = NULL; >> - return 0; >> - } >> - >> - if (bus->iommu_ops != NULL) >> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >> return -EBUSY; >> bus->iommu_ops = ops; > > Do we still need to keep above lines in bus_set_iommu()? It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step. Thanks, Robin.
On 2022/4/19 6:09, Robin Murphy wrote: > On 2022-04-16 01:04, Lu Baolu wrote: >> On 2022/4/14 20:42, Robin Murphy wrote: >>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus) >>> */ >>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) >>> { >>> - int err; >>> - >>> - if (ops == NULL) { >>> - bus->iommu_ops = NULL; >>> - return 0; >>> - } >>> - >>> - if (bus->iommu_ops != NULL) >>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>> return -EBUSY; >>> bus->iommu_ops = ops; >> >> Do we still need to keep above lines in bus_set_iommu()? > > It preserves the existing behaviour until each callsite and its > associated error handling are removed later on, which seems like as good > a thing to do as any. Since I'm already relaxing iommu_device_register() > to a warn-but-continue behaviour while it keeps the bus ops on > life-support internally, I figured not changing too much at once would > make it easier to bisect any potential issues arising from this first step. Fair enough. Thank you for the explanation. Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware. Best regards, baolu
On 2022-04-19 00:37, Lu Baolu wrote: > On 2022/4/19 6:09, Robin Murphy wrote: >> On 2022-04-16 01:04, Lu Baolu wrote: >>> On 2022/4/14 20:42, Robin Murphy wrote: >>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus) >>>> */ >>>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) >>>> { >>>> - int err; >>>> - >>>> - if (ops == NULL) { >>>> - bus->iommu_ops = NULL; >>>> - return 0; >>>> - } >>>> - >>>> - if (bus->iommu_ops != NULL) >>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>> return -EBUSY; >>>> bus->iommu_ops = ops; >>> >>> Do we still need to keep above lines in bus_set_iommu()? >> >> It preserves the existing behaviour until each callsite and its >> associated error handling are removed later on, which seems like as >> good a thing to do as any. Since I'm already relaxing >> iommu_device_register() to a warn-but-continue behaviour while it >> keeps the bus ops on life-support internally, I figured not changing >> too much at once would make it easier to bisect any potential issues >> arising from this first step. > > Fair enough. Thank you for the explanation. > > Do you have a public tree that I could pull these patches and try them > on an Intel hardware? Or perhaps you have done this? I like the whole > idea of this series, but it's better to try it with a real hardware. I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here: https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far. Cheers, Robin.
On 2022/4/19 15:20, Robin Murphy wrote: > On 2022-04-19 00:37, Lu Baolu wrote: >> On 2022/4/19 6:09, Robin Murphy wrote: >>> On 2022-04-16 01:04, Lu Baolu wrote: >>>> On 2022/4/14 20:42, Robin Murphy wrote: >>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type >>>>> *bus) >>>>> */ >>>>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) >>>>> { >>>>> - int err; >>>>> - >>>>> - if (ops == NULL) { >>>>> - bus->iommu_ops = NULL; >>>>> - return 0; >>>>> - } >>>>> - >>>>> - if (bus->iommu_ops != NULL) >>>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>>> return -EBUSY; >>>>> bus->iommu_ops = ops; >>>> >>>> Do we still need to keep above lines in bus_set_iommu()? >>> >>> It preserves the existing behaviour until each callsite and its >>> associated error handling are removed later on, which seems like as >>> good a thing to do as any. Since I'm already relaxing >>> iommu_device_register() to a warn-but-continue behaviour while it >>> keeps the bus ops on life-support internally, I figured not changing >>> too much at once would make it easier to bisect any potential issues >>> arising from this first step. >> >> Fair enough. Thank you for the explanation. >> >> Do you have a public tree that I could pull these patches and try them >> on an Intel hardware? Or perhaps you have done this? I like the whole >> idea of this series, but it's better to try it with a real hardware. > > I haven't bothered with separate branches since there's so many > different pieces in-flight, but my complete (unstable) development > branch can be found here: > > https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus > > For now I'd recommend winding the head back to "iommu: Clean up > bus_set_iommu()" for testing - some of the patches above that have > already been posted and picked up by their respective subsystems, but > others are incomplete and barely compile-tested. I'll probably rearrange > it later this week to better reflect what's happened so far. Okay, thanks for sharing. Best regards, baolu
Good effort to isolate bus config from smmu drivers. Reviewed-By: Krishna Reddy <vdumpa@nvidia.com> I have an orthogonal question here. Can the following code handle the case, where different buses have different type of SMMU instances(like one bus has SMMUv2 and another bus has SMMUv3)? If it need to handle the above case, can the smmu device bus be matched with specific bus here and ops set only for that bus? > + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { > + struct bus_type *bus = iommu_buses[i]; > + const struct iommu_ops *bus_ops = bus->iommu_ops; > + int err; > + > + WARN_ON(bus_ops && bus_ops != ops); > + bus->iommu_ops = ops; > + err = bus_iommu_probe(bus); > + if (err) { > + bus_for_each_dev(bus, NULL, iommu, > remove_iommu_group); > + bus->iommu_ops = bus_ops; > + return err; > + } > + } -KR
On 2022-04-22 19:37, Krishna Reddy wrote: > Good effort to isolate bus config from smmu drivers. > Reviewed-By: Krishna Reddy <vdumpa@nvidia.com> Thanks! > I have an orthogonal question here. > Can the following code handle the case, where different buses have different type of SMMU instances(like one bus has SMMUv2 and another bus has SMMUv3)? > If it need to handle the above case, can the smmu device bus be matched with specific bus here and ops set only for that bus? Not yet, but that is one of the end goals that this is all working towards. I think the stuff that I've added to the dev branch[1] today should have reached the point where that becomes viable, but I'll need to rig up a system to test it next week. Intermediate solutions aren't worth it because in practice you inevitably end up needing both IOMMU drivers to share the platform "bus" anyway. Cheers, Robin. [1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus > > >> + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { >> + struct bus_type *bus = iommu_buses[i]; >> + const struct iommu_ops *bus_ops = bus->iommu_ops; >> + int err; >> + >> + WARN_ON(bus_ops && bus_ops != ops); >> + bus->iommu_ops = ops; >> + err = bus_iommu_probe(bus); >> + if (err) { >> + bus_for_each_dev(bus, NULL, iommu, >> remove_iommu_group); >> + bus->iommu_ops = bus_ops; >> + return err; >> + } >> + } > > > -KR
Hi Robin, On 2022/4/19 15:20, Robin Murphy wrote: > On 2022-04-19 00:37, Lu Baolu wrote: >> On 2022/4/19 6:09, Robin Murphy wrote: >>> On 2022-04-16 01:04, Lu Baolu wrote: >>>> On 2022/4/14 20:42, Robin Murphy wrote: >>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type >>>>> *bus) >>>>> */ >>>>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) >>>>> { >>>>> - int err; >>>>> - >>>>> - if (ops == NULL) { >>>>> - bus->iommu_ops = NULL; >>>>> - return 0; >>>>> - } >>>>> - >>>>> - if (bus->iommu_ops != NULL) >>>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>>> return -EBUSY; >>>>> bus->iommu_ops = ops; >>>> >>>> Do we still need to keep above lines in bus_set_iommu()? >>> >>> It preserves the existing behaviour until each callsite and its >>> associated error handling are removed later on, which seems like as >>> good a thing to do as any. Since I'm already relaxing >>> iommu_device_register() to a warn-but-continue behaviour while it >>> keeps the bus ops on life-support internally, I figured not changing >>> too much at once would make it easier to bisect any potential issues >>> arising from this first step. >> >> Fair enough. Thank you for the explanation. >> >> Do you have a public tree that I could pull these patches and try them >> on an Intel hardware? Or perhaps you have done this? I like the whole >> idea of this series, but it's better to try it with a real hardware. > > I haven't bothered with separate branches since there's so many > different pieces in-flight, but my complete (unstable) development > branch can be found here: > > https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus > > For now I'd recommend winding the head back to "iommu: Clean up > bus_set_iommu()" for testing - some of the patches above that have > already been posted and picked up by their respective subsystems, but > others are incomplete and barely compile-tested. I'll probably rearrange > it later this week to better reflect what's happened so far. I wound the head back to "iommu: Clean up bus_set_iommu" and tested it on an Intel machine. It got stuck during boot. This test was on a remote machine and I have no means to access it physically. So I can't get any kernel debugging messages. (I have to work from home these days. :-() I guess it's due to the fact that intel_iommu_probe_device() callback only works for the pci devices. The issue occurs when probing a device other than a PCI one. Best regards, baolu
On 2022-04-23 09:01, Lu Baolu wrote: > Hi Robin, > > On 2022/4/19 15:20, Robin Murphy wrote: >> On 2022-04-19 00:37, Lu Baolu wrote: >>> On 2022/4/19 6:09, Robin Murphy wrote: >>>> On 2022-04-16 01:04, Lu Baolu wrote: >>>>> On 2022/4/14 20:42, Robin Murphy wrote: >>>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type >>>>>> *bus) >>>>>> */ >>>>>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops >>>>>> *ops) >>>>>> { >>>>>> - int err; >>>>>> - >>>>>> - if (ops == NULL) { >>>>>> - bus->iommu_ops = NULL; >>>>>> - return 0; >>>>>> - } >>>>>> - >>>>>> - if (bus->iommu_ops != NULL) >>>>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>>>> return -EBUSY; >>>>>> bus->iommu_ops = ops; >>>>> >>>>> Do we still need to keep above lines in bus_set_iommu()? >>>> >>>> It preserves the existing behaviour until each callsite and its >>>> associated error handling are removed later on, which seems like as >>>> good a thing to do as any. Since I'm already relaxing >>>> iommu_device_register() to a warn-but-continue behaviour while it >>>> keeps the bus ops on life-support internally, I figured not changing >>>> too much at once would make it easier to bisect any potential issues >>>> arising from this first step. >>> >>> Fair enough. Thank you for the explanation. >>> >>> Do you have a public tree that I could pull these patches and try them >>> on an Intel hardware? Or perhaps you have done this? I like the whole >>> idea of this series, but it's better to try it with a real hardware. >> >> I haven't bothered with separate branches since there's so many >> different pieces in-flight, but my complete (unstable) development >> branch can be found here: >> >> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus >> >> For now I'd recommend winding the head back to "iommu: Clean up >> bus_set_iommu()" for testing - some of the patches above that have >> already been posted and picked up by their respective subsystems, but >> others are incomplete and barely compile-tested. I'll probably >> rearrange it later this week to better reflect what's happened so far. > > I wound the head back to "iommu: Clean up bus_set_iommu" and tested it > on an Intel machine. It got stuck during boot. This test was on a remote > machine and I have no means to access it physically. So I can't get any > kernel debugging messages. (I have to work from home these days. :-() > > I guess it's due to the fact that intel_iommu_probe_device() callback > only works for the pci devices. The issue occurs when probing a device > other than a PCI one. Yeah, I was wondering if that would be significant, since it's the only driver that never registered itself for platform_bus_type so won't have actually seen those calls before. I'm inclined to bodge that as below for now, as long as it then works OK in terms of the rest of the changes. Thanks, Robin. ----->8----- diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 9fa1b98186a3..6e359f92ec00 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4565,6 +4565,10 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev) unsigned long flags; u8 bus, devfn; + /* ANDD platform device support needs fixing */ + if (!pdev) + return ERR_PTR(-ENODEV); + iommu = device_to_iommu(dev, &bus, &devfn); if (!iommu) return ERR_PTR(-ENODEV);
On 2022/4/23 16:37, Robin Murphy wrote: > On 2022-04-23 09:01, Lu Baolu wrote: >> Hi Robin, >> >> On 2022/4/19 15:20, Robin Murphy wrote: >>> On 2022-04-19 00:37, Lu Baolu wrote: >>>> On 2022/4/19 6:09, Robin Murphy wrote: >>>>> On 2022-04-16 01:04, Lu Baolu wrote: >>>>>> On 2022/4/14 20:42, Robin Murphy wrote: >>>>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type >>>>>>> *bus) >>>>>>> */ >>>>>>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops >>>>>>> *ops) >>>>>>> { >>>>>>> - int err; >>>>>>> - >>>>>>> - if (ops == NULL) { >>>>>>> - bus->iommu_ops = NULL; >>>>>>> - return 0; >>>>>>> - } >>>>>>> - >>>>>>> - if (bus->iommu_ops != NULL) >>>>>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>>>>> return -EBUSY; >>>>>>> bus->iommu_ops = ops; >>>>>> >>>>>> Do we still need to keep above lines in bus_set_iommu()? >>>>> >>>>> It preserves the existing behaviour until each callsite and its >>>>> associated error handling are removed later on, which seems like as >>>>> good a thing to do as any. Since I'm already relaxing >>>>> iommu_device_register() to a warn-but-continue behaviour while it >>>>> keeps the bus ops on life-support internally, I figured not >>>>> changing too much at once would make it easier to bisect any >>>>> potential issues arising from this first step. >>>> >>>> Fair enough. Thank you for the explanation. >>>> >>>> Do you have a public tree that I could pull these patches and try them >>>> on an Intel hardware? Or perhaps you have done this? I like the whole >>>> idea of this series, but it's better to try it with a real hardware. >>> >>> I haven't bothered with separate branches since there's so many >>> different pieces in-flight, but my complete (unstable) development >>> branch can be found here: >>> >>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus >>> >>> For now I'd recommend winding the head back to "iommu: Clean up >>> bus_set_iommu()" for testing - some of the patches above that have >>> already been posted and picked up by their respective subsystems, but >>> others are incomplete and barely compile-tested. I'll probably >>> rearrange it later this week to better reflect what's happened so far. >> >> I wound the head back to "iommu: Clean up bus_set_iommu" and tested it >> on an Intel machine. It got stuck during boot. This test was on a remote >> machine and I have no means to access it physically. So I can't get any >> kernel debugging messages. (I have to work from home these days. :-() >> >> I guess it's due to the fact that intel_iommu_probe_device() callback >> only works for the pci devices. The issue occurs when probing a device >> other than a PCI one. > > Yeah, I was wondering if that would be significant, since it's the only > driver that never registered itself for platform_bus_type so won't have > actually seen those calls before. I'm inclined to bodge that as below > for now, as long as it then works OK in terms of the rest of the changes. > > Thanks, > Robin. > > ----->8----- > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 9fa1b98186a3..6e359f92ec00 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -4565,6 +4565,10 @@ static struct iommu_device > *intel_iommu_probe_device(struct device *dev) > unsigned long flags; > u8 bus, devfn; > > + /* ANDD platform device support needs fixing */ > + if (!pdev) > + return ERR_PTR(-ENODEV); > + > iommu = device_to_iommu(dev, &bus, &devfn); > if (!iommu) > return ERR_PTR(-ENODEV); I haven't seen any real ANDD platform devices, hence this works for me. Best regards, baolu
On 2022/4/23 16:51, Lu Baolu wrote: > On 2022/4/23 16:37, Robin Murphy wrote: >> On 2022-04-23 09:01, Lu Baolu wrote: >>> Hi Robin, >>> >>> On 2022/4/19 15:20, Robin Murphy wrote: >>>> On 2022-04-19 00:37, Lu Baolu wrote: >>>>> On 2022/4/19 6:09, Robin Murphy wrote: >>>>>> On 2022-04-16 01:04, Lu Baolu wrote: >>>>>>> On 2022/4/14 20:42, Robin Murphy wrote: >>>>>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct >>>>>>>> bus_type *bus) >>>>>>>> */ >>>>>>>> int bus_set_iommu(struct bus_type *bus, const struct iommu_ops >>>>>>>> *ops) >>>>>>>> { >>>>>>>> - int err; >>>>>>>> - >>>>>>>> - if (ops == NULL) { >>>>>>>> - bus->iommu_ops = NULL; >>>>>>>> - return 0; >>>>>>>> - } >>>>>>>> - >>>>>>>> - if (bus->iommu_ops != NULL) >>>>>>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>>>>>> return -EBUSY; >>>>>>>> bus->iommu_ops = ops; >>>>>>> >>>>>>> Do we still need to keep above lines in bus_set_iommu()? >>>>>> >>>>>> It preserves the existing behaviour until each callsite and its >>>>>> associated error handling are removed later on, which seems like >>>>>> as good a thing to do as any. Since I'm already relaxing >>>>>> iommu_device_register() to a warn-but-continue behaviour while it >>>>>> keeps the bus ops on life-support internally, I figured not >>>>>> changing too much at once would make it easier to bisect any >>>>>> potential issues arising from this first step. >>>>> >>>>> Fair enough. Thank you for the explanation. >>>>> >>>>> Do you have a public tree that I could pull these patches and try them >>>>> on an Intel hardware? Or perhaps you have done this? I like the whole >>>>> idea of this series, but it's better to try it with a real hardware. >>>> >>>> I haven't bothered with separate branches since there's so many >>>> different pieces in-flight, but my complete (unstable) development >>>> branch can be found here: >>>> >>>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus >>>> >>>> For now I'd recommend winding the head back to "iommu: Clean up >>>> bus_set_iommu()" for testing - some of the patches above that have >>>> already been posted and picked up by their respective subsystems, >>>> but others are incomplete and barely compile-tested. I'll probably >>>> rearrange it later this week to better reflect what's happened so far. >>> >>> I wound the head back to "iommu: Clean up bus_set_iommu" and tested it >>> on an Intel machine. It got stuck during boot. This test was on a remote >>> machine and I have no means to access it physically. So I can't get any >>> kernel debugging messages. (I have to work from home these days. :-() >>> >>> I guess it's due to the fact that intel_iommu_probe_device() callback >>> only works for the pci devices. The issue occurs when probing a device >>> other than a PCI one. >> >> Yeah, I was wondering if that would be significant, since it's the >> only driver that never registered itself for platform_bus_type so >> won't have actually seen those calls before. I'm inclined to bodge >> that as below for now, as long as it then works OK in terms of the >> rest of the changes. >> >> Thanks, >> Robin. >> >> ----->8----- >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >> index 9fa1b98186a3..6e359f92ec00 100644 >> --- a/drivers/iommu/intel/iommu.c >> +++ b/drivers/iommu/intel/iommu.c >> @@ -4565,6 +4565,10 @@ static struct iommu_device >> *intel_iommu_probe_device(struct device *dev) >> unsigned long flags; >> u8 bus, devfn; >> >> + /* ANDD platform device support needs fixing */ >> + if (!pdev) >> + return ERR_PTR(-ENODEV); >> + >> iommu = device_to_iommu(dev, &bus, &devfn); >> if (!iommu) >> return ERR_PTR(-ENODEV); > > I haven't seen any real ANDD platform devices, hence this works for me. Or more precisely, return NULL when @dev goes through device_to_iommu()? diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index df5c62ecf942..0d447739e076 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -797,8 +797,11 @@ struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn) pf_pdev = pci_physfn(pdev); dev = &pf_pdev->dev; segment = pci_domain_nr(pdev->bus); - } else if (has_acpi_companion(dev)) + } else if (has_acpi_companion(dev)) { dev = &ACPI_COMPANION(dev)->dev; + } else { + return NULL; + } rcu_read_lock(); for_each_iommu(iommu, drhd) { Best regards, baolu
On 2022/4/23 17:00, Lu Baolu wrote: > On 2022/4/23 16:51, Lu Baolu wrote: >> On 2022/4/23 16:37, Robin Murphy wrote: >>> On 2022-04-23 09:01, Lu Baolu wrote: >>>> Hi Robin, >>>> >>>> On 2022/4/19 15:20, Robin Murphy wrote: >>>>> On 2022-04-19 00:37, Lu Baolu wrote: >>>>>> On 2022/4/19 6:09, Robin Murphy wrote: >>>>>>> On 2022-04-16 01:04, Lu Baolu wrote: >>>>>>>> On 2022/4/14 20:42, Robin Murphy wrote: >>>>>>>>> @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct >>>>>>>>> bus_type *bus) >>>>>>>>> */ >>>>>>>>> int bus_set_iommu(struct bus_type *bus, const struct >>>>>>>>> iommu_ops *ops) >>>>>>>>> { >>>>>>>>> - int err; >>>>>>>>> - >>>>>>>>> - if (ops == NULL) { >>>>>>>>> - bus->iommu_ops = NULL; >>>>>>>>> - return 0; >>>>>>>>> - } >>>>>>>>> - >>>>>>>>> - if (bus->iommu_ops != NULL) >>>>>>>>> + if (bus->iommu_ops && ops && bus->iommu_ops != ops) >>>>>>>>> return -EBUSY; >>>>>>>>> bus->iommu_ops = ops; >>>>>>>> >>>>>>>> Do we still need to keep above lines in bus_set_iommu()? >>>>>>> >>>>>>> It preserves the existing behaviour until each callsite and its >>>>>>> associated error handling are removed later on, which seems like >>>>>>> as good a thing to do as any. Since I'm already relaxing >>>>>>> iommu_device_register() to a warn-but-continue behaviour while it >>>>>>> keeps the bus ops on life-support internally, I figured not >>>>>>> changing too much at once would make it easier to bisect any >>>>>>> potential issues arising from this first step. >>>>>> >>>>>> Fair enough. Thank you for the explanation. >>>>>> >>>>>> Do you have a public tree that I could pull these patches and try >>>>>> them >>>>>> on an Intel hardware? Or perhaps you have done this? I like the whole >>>>>> idea of this series, but it's better to try it with a real hardware. >>>>> >>>>> I haven't bothered with separate branches since there's so many >>>>> different pieces in-flight, but my complete (unstable) development >>>>> branch can be found here: >>>>> >>>>> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus >>>>> >>>>> For now I'd recommend winding the head back to "iommu: Clean up >>>>> bus_set_iommu()" for testing - some of the patches above that have >>>>> already been posted and picked up by their respective subsystems, >>>>> but others are incomplete and barely compile-tested. I'll probably >>>>> rearrange it later this week to better reflect what's happened so far. >>>> >>>> I wound the head back to "iommu: Clean up bus_set_iommu" and tested it >>>> on an Intel machine. It got stuck during boot. This test was on a >>>> remote >>>> machine and I have no means to access it physically. So I can't get any >>>> kernel debugging messages. (I have to work from home these days. :-() >>>> >>>> I guess it's due to the fact that intel_iommu_probe_device() callback >>>> only works for the pci devices. The issue occurs when probing a device >>>> other than a PCI one. >>> >>> Yeah, I was wondering if that would be significant, since it's the >>> only driver that never registered itself for platform_bus_type so >>> won't have actually seen those calls before. I'm inclined to bodge >>> that as below for now, as long as it then works OK in terms of the >>> rest of the changes. >>> >>> Thanks, >>> Robin. >>> >>> ----->8----- >>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >>> index 9fa1b98186a3..6e359f92ec00 100644 >>> --- a/drivers/iommu/intel/iommu.c >>> +++ b/drivers/iommu/intel/iommu.c >>> @@ -4565,6 +4565,10 @@ static struct iommu_device >>> *intel_iommu_probe_device(struct device *dev) >>> unsigned long flags; >>> u8 bus, devfn; >>> >>> + /* ANDD platform device support needs fixing */ >>> + if (!pdev) >>> + return ERR_PTR(-ENODEV); >>> + >>> iommu = device_to_iommu(dev, &bus, &devfn); >>> if (!iommu) >>> return ERR_PTR(-ENODEV); >> >> I haven't seen any real ANDD platform devices, hence this works for me. > > Or more precisely, return NULL when @dev goes through device_to_iommu()? > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index df5c62ecf942..0d447739e076 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -797,8 +797,11 @@ struct intel_iommu *device_to_iommu(struct device > *dev, u8 *bus, u8 *devfn) > pf_pdev = pci_physfn(pdev); > dev = &pf_pdev->dev; > segment = pci_domain_nr(pdev->bus); > - } else if (has_acpi_companion(dev)) > + } else if (has_acpi_companion(dev)) { > dev = &ACPI_COMPANION(dev)->dev; > + } else { > + return NULL; > + } > > rcu_read_lock(); > for_each_iommu(iommu, drhd) { Robin, please ignore this. "has_acpi_companion(dev)" isn't equal to an ANDD device. Please use yours. Sorry for the noise. Best regards, baolu
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 13e1a8bd5435..51205c33c426 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -175,6 +175,14 @@ static int __init iommu_subsys_init(void) } subsys_initcall(iommu_subsys_init); +static int remove_iommu_group(struct device *dev, void *data) +{ + if (dev->iommu && dev->iommu->iommu_dev == data) + iommu_release_device(dev); + + return 0; +} + /** * iommu_device_register() - Register an IOMMU hardware instance * @iommu: IOMMU handle for the instance @@ -197,6 +205,22 @@ int iommu_device_register(struct iommu_device *iommu, spin_lock(&iommu_device_lock); list_add_tail(&iommu->list, &iommu_device_list); spin_unlock(&iommu_device_lock); + + for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) { + struct bus_type *bus = iommu_buses[i]; + const struct iommu_ops *bus_ops = bus->iommu_ops; + int err; + + WARN_ON(bus_ops && bus_ops != ops); + bus->iommu_ops = ops; + err = bus_iommu_probe(bus); + if (err) { + bus_for_each_dev(bus, NULL, iommu, remove_iommu_group); + bus->iommu_ops = bus_ops; + return err; + } + } + return 0; } EXPORT_SYMBOL_GPL(iommu_device_register); @@ -1654,13 +1678,6 @@ static int probe_iommu_group(struct device *dev, void *data) return ret; } -static int remove_iommu_group(struct device *dev, void *data) -{ - iommu_release_device(dev); - - return 0; -} - static int iommu_bus_notifier(struct notifier_block *nb, unsigned long action, void *data) { @@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus) */ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) { - int err; - - if (ops == NULL) { - bus->iommu_ops = NULL; - return 0; - } - - if (bus->iommu_ops != NULL) + if (bus->iommu_ops && ops && bus->iommu_ops != ops) return -EBUSY; bus->iommu_ops = ops; - /* Do IOMMU specific setup for this bus-type */ - err = bus_iommu_probe(bus); - if (err) { - /* Clean up */ - bus_for_each_dev(bus, NULL, NULL, remove_iommu_group); - bus->iommu_ops = NULL; - } - - return err; + return 0; } EXPORT_SYMBOL_GPL(bus_set_iommu);
Move the bus setup to iommu_device_register(). This should allow bus_iommu_probe() to be correctly replayed for multiple IOMMU instances, and leaves bus_set_iommu() as a glorfied no-op to be cleaned up next. Note that although the handling of errors from bus_iommu_probe() looks inadequate, it is merely preserving the well-established existing behaviour. This could be improved in future - probably combined with equivalent cleanup for iommu_device_unregister() - but that isn't a priority right now. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/iommu.c | 50 ++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 24 deletions(-)