Re: [PATCH 1/3] iommu/vt-d:Add support for detecting ACPI device in RMRR

2020-08-26 Thread Dan Carpenter
On Wed, Aug 26, 2020 at 06:27:50AM -0400, FelixCuioc wrote:
> Some ACPI devices need to issue dma requests to access
> the reserved memory area.BIOS uses the device scope type
> ACPI_NAMESPACE_DEVICE in RMRR to report these ACPI devices.
> This patch add support for detecting ACPI devices in RMRR.
> 
> Signed-off-by: FelixCuioc 
> ---
>  drivers/iommu/intel/dmar.c  | 74 -
>  drivers/iommu/intel/iommu.c | 22 ++-
>  include/linux/dmar.h| 12 +-
>  3 files changed, 72 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 93e6345f3414..024ca38dba12 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -215,7 +215,7 @@ static bool dmar_match_pci_path(struct 
> dmar_pci_notify_info *info, int bus,
>  }
>  
>  /* Return: > 0 if match found, 0 if no match found, < 0 if error happens */
> -int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
> +int dmar_pci_insert_dev_scope(struct dmar_pci_notify_info *info,
> void *start, void*end, u16 segment,
> struct dmar_dev_scope *devices,
> int devices_cnt)
> @@ -304,7 +304,7 @@ static int dmar_pci_bus_add_dev(struct 
> dmar_pci_notify_info *info)
>  
>   drhd = container_of(dmaru->hdr,
>   struct acpi_dmar_hardware_unit, header);
> - ret = dmar_insert_dev_scope(info, (void *)(drhd + 1),
> + ret = dmar_pci_insert_dev_scope(info, (void *)(drhd + 1),
>   ((void *)drhd) + drhd->header.length,
>   dmaru->segment,
>   dmaru->devices, dmaru->devices_cnt);
> @@ -696,48 +696,56 @@ dmar_find_matched_drhd_unit(struct pci_dev *dev)
>  
>   return dmaru;
>  }
> -
> -static void __init dmar_acpi_insert_dev_scope(u8 device_number,
> -   struct acpi_device *adev)
> +int dmar_acpi_insert_dev_scope(u8 device_number,

The patch deleted the blank line between functions.  Make this function
bool, change the 1/0 to true/false.  Add a comment explaining what the
return values mean.

> + struct acpi_device *adev,
> + void *start, void *end,
> + struct dmar_dev_scope *devices,
> + int devices_cnt)
>  {
> - struct dmar_drhd_unit *dmaru;
> - struct acpi_dmar_hardware_unit *drhd;
>   struct acpi_dmar_device_scope *scope;
>   struct device *tmp;
>   int i;
>   struct acpi_dmar_pci_path *path;
>  
> + for (; start < end; start += scope->length) {
> + scope = start;

Are we sure that there is enough space for sizeof(*scope) in end - start?
(I don't know this code so maybe we are).

> + if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
> + continue;
> + if (scope->enumeration_id != device_number)
> + continue;
> + path = (void *)(scope + 1);
> + for_each_dev_scope(devices, devices_cnt, i, tmp)
> + if (tmp == NULL) {
> + devices[i].bus = scope->bus;
> + devices[i].devfn = PCI_DEVFN(path->device, 
> path->function);
> + rcu_assign_pointer(devices[i].dev,
> +get_device(>dev));
> + return 1;
> + }
> + WARN_ON(i >= devices_cnt);
> + }
> + return 0;
> +}
> +static int dmar_acpi_bus_add_dev(u8 device_number, struct acpi_device *adev)
> +{
> + struct dmar_drhd_unit *dmaru;
> + struct acpi_dmar_hardware_unit *drhd;
> + int ret = 0;

This initialization is never used.

> +
>   for_each_drhd_unit(dmaru) {
>   drhd = container_of(dmaru->hdr,
>   struct acpi_dmar_hardware_unit,
>   header);
> + ret = dmar_acpi_insert_dev_scope(device_number, adev, (void 
> *)(drhd+1),
> + ((void 
> *)drhd)+drhd->header.length,
> + dmaru->devices, 
> dmaru->devices_cnt);
> + if (ret)
> + break;
> + }
> + ret = dmar_rmrr_add_acpi_dev(device_number, adev);

What about if dmar_acpi_insert_dev_scope() always returns zero, do we
still want to call dmar_rmrr_add_acpi_dev()?

>  
> - for (scope = (void *)(drhd + 1);
> -  (unsigned long)scope < ((unsigned long)drhd) + 
> drhd->header.length;
> -  scope = ((void *)scope) + scope->length) {
> - if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
> - continue;
> - if 

[PATCH 1/3] iommu/vt-d:Add support for detecting ACPI device in RMRR

2020-08-26 Thread FelixCuioc
Some ACPI devices need to issue dma requests to access
the reserved memory area.BIOS uses the device scope type
ACPI_NAMESPACE_DEVICE in RMRR to report these ACPI devices.
This patch add support for detecting ACPI devices in RMRR.

Signed-off-by: FelixCuioc 
---
 drivers/iommu/intel/dmar.c  | 74 -
 drivers/iommu/intel/iommu.c | 22 ++-
 include/linux/dmar.h| 12 +-
 3 files changed, 72 insertions(+), 36 deletions(-)

diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
index 93e6345f3414..024ca38dba12 100644
--- a/drivers/iommu/intel/dmar.c
+++ b/drivers/iommu/intel/dmar.c
@@ -215,7 +215,7 @@ static bool dmar_match_pci_path(struct dmar_pci_notify_info 
*info, int bus,
 }
 
 /* Return: > 0 if match found, 0 if no match found, < 0 if error happens */
-int dmar_insert_dev_scope(struct dmar_pci_notify_info *info,
+int dmar_pci_insert_dev_scope(struct dmar_pci_notify_info *info,
  void *start, void*end, u16 segment,
  struct dmar_dev_scope *devices,
  int devices_cnt)
@@ -304,7 +304,7 @@ static int dmar_pci_bus_add_dev(struct dmar_pci_notify_info 
*info)
 
drhd = container_of(dmaru->hdr,
struct acpi_dmar_hardware_unit, header);
-   ret = dmar_insert_dev_scope(info, (void *)(drhd + 1),
+   ret = dmar_pci_insert_dev_scope(info, (void *)(drhd + 1),
((void *)drhd) + drhd->header.length,
dmaru->segment,
dmaru->devices, dmaru->devices_cnt);
@@ -696,48 +696,56 @@ dmar_find_matched_drhd_unit(struct pci_dev *dev)
 
return dmaru;
 }
-
-static void __init dmar_acpi_insert_dev_scope(u8 device_number,
- struct acpi_device *adev)
+int dmar_acpi_insert_dev_scope(u8 device_number,
+   struct acpi_device *adev,
+   void *start, void *end,
+   struct dmar_dev_scope *devices,
+   int devices_cnt)
 {
-   struct dmar_drhd_unit *dmaru;
-   struct acpi_dmar_hardware_unit *drhd;
struct acpi_dmar_device_scope *scope;
struct device *tmp;
int i;
struct acpi_dmar_pci_path *path;
 
+   for (; start < end; start += scope->length) {
+   scope = start;
+   if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
+   continue;
+   if (scope->enumeration_id != device_number)
+   continue;
+   path = (void *)(scope + 1);
+   for_each_dev_scope(devices, devices_cnt, i, tmp)
+   if (tmp == NULL) {
+   devices[i].bus = scope->bus;
+   devices[i].devfn = PCI_DEVFN(path->device, 
path->function);
+   rcu_assign_pointer(devices[i].dev,
+  get_device(>dev));
+   return 1;
+   }
+   WARN_ON(i >= devices_cnt);
+   }
+   return 0;
+}
+static int dmar_acpi_bus_add_dev(u8 device_number, struct acpi_device *adev)
+{
+   struct dmar_drhd_unit *dmaru;
+   struct acpi_dmar_hardware_unit *drhd;
+   int ret = 0;
+
for_each_drhd_unit(dmaru) {
drhd = container_of(dmaru->hdr,
struct acpi_dmar_hardware_unit,
header);
+   ret = dmar_acpi_insert_dev_scope(device_number, adev, (void 
*)(drhd+1),
+   ((void 
*)drhd)+drhd->header.length,
+   dmaru->devices, 
dmaru->devices_cnt);
+   if (ret)
+   break;
+   }
+   ret = dmar_rmrr_add_acpi_dev(device_number, adev);
 
-   for (scope = (void *)(drhd + 1);
-(unsigned long)scope < ((unsigned long)drhd) + 
drhd->header.length;
-scope = ((void *)scope) + scope->length) {
-   if (scope->entry_type != ACPI_DMAR_SCOPE_TYPE_NAMESPACE)
-   continue;
-   if (scope->enumeration_id != device_number)
-   continue;
+   return ret;
 
-   path = (void *)(scope + 1);
-   pr_info("ACPI device \"%s\" under DMAR at %llx as 
%02x:%02x.%d\n",
-   dev_name(>dev), dmaru->reg_base_addr,
-   scope->bus, path->device, path->function);
-   for_each_dev_scope(dmaru->devices, dmaru->devices_cnt, 
i, tmp)
-   if (tmp == NULL) {
-   dmaru->devices[i].bus =