thr3ads.net - Xen devel - [Xen-devel] [PATCH 1/2] VT-d: print message cleanup [Dec 2009]

If this information is useful, please help other people find it:
Share via:

Han, Weidong

2009-Dec-02 05:48 UTC

[Xen-devel] [PATCH 1/2] VT-d: print message cleanup

This patch use some indention to make VT-d parsing messages more readable in
dmar.c file.

Signed-off-by: Weidong Han <weidong.han@intel.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman

2009-Dec-02 21:16 UTC

head link

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

On Wed, Dec 02, 2009 at 01:48:23PM +0800, Han, Weidong
wrote:> This patch use some indention to make VT-d parsing messages more readable
in dmar.c file.
> 
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman

2009-Dec-02 21:16 UTC

head link

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

On Thu, Dec 03, 2009 at 08:16:16AM +1100, Simon Horman
wrote:> On Wed, Dec 02, 2009 at 01:48:23PM +0800, Han, Weidong wrote:
> > This patch use some indention to make VT-d parsing messages more
readable in dmar.c file.
> > 
> > Signed-off-by: Weidong Han <weidong.han@intel.com>
> > 
Sorry, miss-fire. I''ll repost.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman

2009-Dec-02 21:17 UTC

head link

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

On Wed, Dec 02, 2009 at 01:48:23PM +0800, Han, Weidong
wrote:> This patch use some indention to make VT-d parsing messages more readable
in dmar.c file.
Good idea, but there are several changes below that
don''t seem to relation to indentation. They really
ought to be in separate patches or at the least
noted in the changelog.
> 
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> 
> diff -r eb6c6d9464b4 xen/drivers/passthrough/vtd/dmar.c
> --- a/xen/drivers/passthrough/vtd/dmar.c	Wed Dec 02 04:03:55 2009 +0800
> +++ b/xen/drivers/passthrough/vtd/dmar.c	Wed Dec 02 06:56:53 2009 +0800
> @@ -309,7 +309,7 @@ static int __init acpi_parse_dev_scope(v
>              sub_bus = pci_conf_read8(
>                  bus, path->dev, path->fn, PCI_SUBORDINATE_BUS);
>              dprintk(XENLOG_INFO VTDPREFIX,
> -                    "bridge: %x:%x.%x  start = %x sec = %x  sub =
%x\n",
> +                    "  bridge: %x:%x.%x  start = %x sec = %x  sub =
%x\n",
>                      bus, path->dev, path->fn,
>                      acpi_scope->start_bus, sec_bus, sub_bus);
>  
> @@ -317,17 +317,17 @@ static int __init acpi_parse_dev_scope(v
>              break;
>  
>          case ACPI_DEV_MSI_HPET:
> -            dprintk(XENLOG_INFO VTDPREFIX, "MSI HPET:
%x:%x.%x\n",
> +            dprintk(XENLOG_INFO VTDPREFIX, "  MSI HPET:
%x:%x.%x\n",
>                      bus, path->dev, path->fn);
>              break;
>  
>          case ACPI_DEV_ENDPOINT:
> -            dprintk(XENLOG_INFO VTDPREFIX, "endpoint:
%x:%x.%x\n",
> +            dprintk(XENLOG_INFO VTDPREFIX, "  endpoint:
%x:%x.%x\n",
>                      bus, path->dev, path->fn);
>              break;
>  
>          case ACPI_DEV_IOAPIC:
> -            dprintk(XENLOG_INFO VTDPREFIX, "IOAPIC: %x:%x.%x\n",
> +            dprintk(XENLOG_INFO VTDPREFIX, "  IOAPIC:
%x:%x.%x\n",
>                      bus, path->dev, path->fn);
>  
>              if ( type == DMAR_TYPE )
> @@ -370,7 +370,7 @@ acpi_parse_one_drhd(struct acpi_dmar_ent
>      dmaru->address = drhd->address;
>      dmaru->include_all = drhd->flags & 1; /* BIT0: INCLUDE_ALL
*/
>      INIT_LIST_HEAD(&dmaru->ioapic_list);
> -    dprintk(XENLOG_INFO VTDPREFIX, "dmaru->address =
%"PRIx64"\n",
> +    dprintk(XENLOG_INFO VTDPREFIX, "  dmaru->address =
%"PRIx64"\n",
>              dmaru->address);
>  
>      addr = map_to_nocache_virt(0, drhd->address);
> @@ -383,7 +383,7 @@ acpi_parse_one_drhd(struct acpi_dmar_ent
>  
>      if ( dmaru->include_all )
>      {
> -        dprintk(XENLOG_INFO VTDPREFIX, "found INCLUDE_ALL\n");
> +        dprintk(XENLOG_INFO VTDPREFIX, "  flags:
INCLUDE_ALL\n");
>          /* Only allow one INCLUDE_ALL */
>          if ( include_all )
>          {
> @@ -407,26 +407,30 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
>      struct acpi_table_rmrr *rmrr = (struct acpi_table_rmrr *)header;
>      struct acpi_rmrr_unit *rmrru;
>      void *dev_scope_start, *dev_scope_end;
> +    u64 base_addr = rmrr->base_address, end_addr =
rmrr->end_address;
>      int ret = 0;
       This and everything else that uses base_addr and end_addr
       do not seem to be indentation changes.
>  
> -    if ( rmrr->base_address >= rmrr->end_address )
> +    if ( base_addr >= end_addr )
>      {
>          dprintk(XENLOG_ERR VTDPREFIX,
>                  "RMRR error: base_addr %"PRIx64"
end_address %"PRIx64"\n",
> -                rmrr->base_address, rmrr->end_address);
> +                base_addr, end_addr);
>          return -EFAULT;
>      }
>  
>  #ifdef CONFIG_X86
> -    /* This check is here simply to detect when RMRR values are not
properly represented in the
> -       system memory map and inform the user */
> -    if ( (!page_is_ram_type(paddr_to_pfn(rmrr->base_address),
RAM_TYPE_RESERVED))||
> -         (!page_is_ram_type(paddr_to_pfn(rmrr->end_address) - 1,
RAM_TYPE_RESERVED)) )
> +    /* This check is here simply to detect when RMRR values are
> +     * not properly represented in the system memory map and
> +     * inform the user
> +     */
> +    if ( (!page_is_ram_type(paddr_to_pfn(base_addr), RAM_TYPE_RESERVED))
||
> +         (!page_is_ram_type(paddr_to_pfn(end_addr) - 1,
RAM_TYPE_RESERVED)) )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "RMRR address range not in reserved memory base =
%"PRIx64" end = %"PRIx64"; " \
> +                "  RMRR address range not in reserved memory "
> +                "base = %"PRIx64" end =
%"PRIx64"; "
>                  "iommu_inclusive_mapping=1 parameter may be
needed.\n",
> -                rmrr->base_address, rmrr->end_address);
> +                base_addr, end_addr);
>      }
>  #endif
>  
> @@ -435,8 +439,12 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
>          return -ENOMEM;
>      memset(rmrru, 0, sizeof(struct acpi_rmrr_unit));
>  
> -    rmrru->base_address = rmrr->base_address;
> -    rmrru->end_address = rmrr->end_address;
> +    rmrru->base_address = base_addr;
> +    rmrru->end_address = end_addr;
> +    dprintk(XENLOG_INFO VTDPREFIX,
> +            "  RMRR region: base_addr %"PRIx64" end_address
%"PRIx64"\n",
> +            rmrru->base_address, rmrru->end_address);
> +
       This extra debug statement isn''t an indentation change.
>      dev_scope_start = (void *)(rmrr + 1);
>      dev_scope_end   = ((void *)rmrr) + header->length;
>      ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
> @@ -464,6 +472,8 @@ acpi_parse_one_atsr(struct acpi_dmar_ent
>      memset(atsru, 0, sizeof(struct acpi_atsr_unit));
>  
>      atsru->all_ports = atsr->flags & 1; /* BIT0: ALL_PORTS */
> +    dprintk(XENLOG_INFO VTDPREFIX,
> +            "  atsru->all_ports: %x\n", atsru->all_ports);
	Nor is this one.
>      if ( !atsru->all_ports )
>      {
>          dev_scope_start = (void *)(atsr + 1);
> @@ -473,7 +483,7 @@ acpi_parse_one_atsr(struct acpi_dmar_ent
>      }
>      else
>      {
> -        dprintk(XENLOG_INFO VTDPREFIX, "found ALL_PORTS\n");
> +        dprintk(XENLOG_INFO VTDPREFIX, "  flags: ALL_PORTS\n");
>          /* Only allow one ALL_PORTS */
>          if ( all_ports )
>          {
> @@ -506,6 +516,9 @@ acpi_parse_one_rhsa(struct acpi_dmar_ent
>      rhsau->address = rhsa->address;
>      rhsau->proximity_domain = rhsa->proximity_domain;
>      list_add_tail(&rhsau->list, &acpi_rhsa_units);
> +    dprintk(XENLOG_INFO VTDPREFIX,
> +            "  rhsau->address: %"PRIx64"
rhsau->proximity_domain: %"PRIx32"\n",
> +            rhsau->address, rhsau->proximity_domain);
	Or this one.
>  
>      return ret;
>  }
> @@ -541,19 +554,19 @@ static int __init acpi_parse_dmar(struct
>          switch ( entry_header->type )
>          {
>          case ACPI_DMAR_DRHD:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_DRHD\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_DRHD:\n");
>              ret = acpi_parse_one_drhd(entry_header);
>              break;
>          case ACPI_DMAR_RMRR:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RMRR\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RMRR:\n");
>              ret = acpi_parse_one_rmrr(entry_header);
>              break;
>          case ACPI_DMAR_ATSR:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_ATSR\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_ATSR:\n");
>              ret = acpi_parse_one_atsr(entry_header);
>              break;
>          case ACPI_DMAR_RHSA:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RHSA\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RHSA:\n");
>              ret = acpi_parse_one_rhsa(entry_header);
>              break;
>          default:
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Han, Weidong

2009-Dec-03 02:08 UTC

head link

RE: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

Simon,

Thanks for your careful review.

Yes, the description is too simple. The patch mainly changes indention, but also
does some other cleanup as you pointed below. But Keir has already checked in
the patch.

Regards,
Weidong

-----Original Message-----
From: Simon Horman [mailto:horms@verge.net.au] 
Sent: Thursday, December 03, 2009 5:17 AM
To: Han, Weidong
Cc: xen-devel@lists.xensource.com; Keir Fraser
Subject: Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

On Wed, Dec 02, 2009 at 01:48:23PM +0800, Han, Weidong
wrote:> This patch use some indention to make VT-d parsing messages more readable
in dmar.c file.
Good idea, but there are several changes below that
don''t seem to relation to indentation. They really
ought to be in separate patches or at the least
noted in the changelog.
> 
> Signed-off-by: Weidong Han <weidong.han@intel.com>
> 
> diff -r eb6c6d9464b4 xen/drivers/passthrough/vtd/dmar.c
> --- a/xen/drivers/passthrough/vtd/dmar.c	Wed Dec 02 04:03:55 2009 +0800
> +++ b/xen/drivers/passthrough/vtd/dmar.c	Wed Dec 02 06:56:53 2009 +0800
> @@ -309,7 +309,7 @@ static int __init acpi_parse_dev_scope(v
>              sub_bus = pci_conf_read8(
>                  bus, path->dev, path->fn, PCI_SUBORDINATE_BUS);
>              dprintk(XENLOG_INFO VTDPREFIX,
> -                    "bridge: %x:%x.%x  start = %x sec = %x  sub =
%x\n",
> +                    "  bridge: %x:%x.%x  start = %x sec = %x  sub =
%x\n",
>                      bus, path->dev, path->fn,
>                      acpi_scope->start_bus, sec_bus, sub_bus);
>  
> @@ -317,17 +317,17 @@ static int __init acpi_parse_dev_scope(v
>              break;
>  
>          case ACPI_DEV_MSI_HPET:
> -            dprintk(XENLOG_INFO VTDPREFIX, "MSI HPET:
%x:%x.%x\n",
> +            dprintk(XENLOG_INFO VTDPREFIX, "  MSI HPET:
%x:%x.%x\n",
>                      bus, path->dev, path->fn);
>              break;
>  
>          case ACPI_DEV_ENDPOINT:
> -            dprintk(XENLOG_INFO VTDPREFIX, "endpoint:
%x:%x.%x\n",
> +            dprintk(XENLOG_INFO VTDPREFIX, "  endpoint:
%x:%x.%x\n",
>                      bus, path->dev, path->fn);
>              break;
>  
>          case ACPI_DEV_IOAPIC:
> -            dprintk(XENLOG_INFO VTDPREFIX, "IOAPIC: %x:%x.%x\n",
> +            dprintk(XENLOG_INFO VTDPREFIX, "  IOAPIC:
%x:%x.%x\n",
>                      bus, path->dev, path->fn);
>  
>              if ( type == DMAR_TYPE )
> @@ -370,7 +370,7 @@ acpi_parse_one_drhd(struct acpi_dmar_ent
>      dmaru->address = drhd->address;
>      dmaru->include_all = drhd->flags & 1; /* BIT0: INCLUDE_ALL
*/
>      INIT_LIST_HEAD(&dmaru->ioapic_list);
> -    dprintk(XENLOG_INFO VTDPREFIX, "dmaru->address =
%"PRIx64"\n",
> +    dprintk(XENLOG_INFO VTDPREFIX, "  dmaru->address =
%"PRIx64"\n",
>              dmaru->address);
>  
>      addr = map_to_nocache_virt(0, drhd->address);
> @@ -383,7 +383,7 @@ acpi_parse_one_drhd(struct acpi_dmar_ent
>  
>      if ( dmaru->include_all )
>      {
> -        dprintk(XENLOG_INFO VTDPREFIX, "found INCLUDE_ALL\n");
> +        dprintk(XENLOG_INFO VTDPREFIX, "  flags:
INCLUDE_ALL\n");
>          /* Only allow one INCLUDE_ALL */
>          if ( include_all )
>          {
> @@ -407,26 +407,30 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
>      struct acpi_table_rmrr *rmrr = (struct acpi_table_rmrr *)header;
>      struct acpi_rmrr_unit *rmrru;
>      void *dev_scope_start, *dev_scope_end;
> +    u64 base_addr = rmrr->base_address, end_addr =
rmrr->end_address;
>      int ret = 0;
       This and everything else that uses base_addr and end_addr
       do not seem to be indentation changes.
>  
> -    if ( rmrr->base_address >= rmrr->end_address )
> +    if ( base_addr >= end_addr )
>      {
>          dprintk(XENLOG_ERR VTDPREFIX,
>                  "RMRR error: base_addr %"PRIx64"
end_address %"PRIx64"\n",
> -                rmrr->base_address, rmrr->end_address);
> +                base_addr, end_addr);
>          return -EFAULT;
>      }
>  
>  #ifdef CONFIG_X86
> -    /* This check is here simply to detect when RMRR values are not
properly represented in the
> -       system memory map and inform the user */
> -    if ( (!page_is_ram_type(paddr_to_pfn(rmrr->base_address),
RAM_TYPE_RESERVED))||
> -         (!page_is_ram_type(paddr_to_pfn(rmrr->end_address) - 1,
RAM_TYPE_RESERVED)) )
> +    /* This check is here simply to detect when RMRR values are
> +     * not properly represented in the system memory map and
> +     * inform the user
> +     */
> +    if ( (!page_is_ram_type(paddr_to_pfn(base_addr), RAM_TYPE_RESERVED))
||
> +         (!page_is_ram_type(paddr_to_pfn(end_addr) - 1,
RAM_TYPE_RESERVED)) )
>      {
>          dprintk(XENLOG_WARNING VTDPREFIX,
> -                "RMRR address range not in reserved memory base =
%"PRIx64" end = %"PRIx64"; " \
> +                "  RMRR address range not in reserved memory "
> +                "base = %"PRIx64" end =
%"PRIx64"; "
>                  "iommu_inclusive_mapping=1 parameter may be
needed.\n",
> -                rmrr->base_address, rmrr->end_address);
> +                base_addr, end_addr);
>      }
>  #endif
>  
> @@ -435,8 +439,12 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
>          return -ENOMEM;
>      memset(rmrru, 0, sizeof(struct acpi_rmrr_unit));
>  
> -    rmrru->base_address = rmrr->base_address;
> -    rmrru->end_address = rmrr->end_address;
> +    rmrru->base_address = base_addr;
> +    rmrru->end_address = end_addr;
> +    dprintk(XENLOG_INFO VTDPREFIX,
> +            "  RMRR region: base_addr %"PRIx64" end_address
%"PRIx64"\n",
> +            rmrru->base_address, rmrru->end_address);
> +
       This extra debug statement isn''t an indentation change.
>      dev_scope_start = (void *)(rmrr + 1);
>      dev_scope_end   = ((void *)rmrr) + header->length;
>      ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
> @@ -464,6 +472,8 @@ acpi_parse_one_atsr(struct acpi_dmar_ent
>      memset(atsru, 0, sizeof(struct acpi_atsr_unit));
>  
>      atsru->all_ports = atsr->flags & 1; /* BIT0: ALL_PORTS */
> +    dprintk(XENLOG_INFO VTDPREFIX,
> +            "  atsru->all_ports: %x\n", atsru->all_ports);
	Nor is this one.
>      if ( !atsru->all_ports )
>      {
>          dev_scope_start = (void *)(atsr + 1);
> @@ -473,7 +483,7 @@ acpi_parse_one_atsr(struct acpi_dmar_ent
>      }
>      else
>      {
> -        dprintk(XENLOG_INFO VTDPREFIX, "found ALL_PORTS\n");
> +        dprintk(XENLOG_INFO VTDPREFIX, "  flags: ALL_PORTS\n");
>          /* Only allow one ALL_PORTS */
>          if ( all_ports )
>          {
> @@ -506,6 +516,9 @@ acpi_parse_one_rhsa(struct acpi_dmar_ent
>      rhsau->address = rhsa->address;
>      rhsau->proximity_domain = rhsa->proximity_domain;
>      list_add_tail(&rhsau->list, &acpi_rhsa_units);
> +    dprintk(XENLOG_INFO VTDPREFIX,
> +            "  rhsau->address: %"PRIx64"
rhsau->proximity_domain: %"PRIx32"\n",
> +            rhsau->address, rhsau->proximity_domain);
	Or this one.
>  
>      return ret;
>  }
> @@ -541,19 +554,19 @@ static int __init acpi_parse_dmar(struct
>          switch ( entry_header->type )
>          {
>          case ACPI_DMAR_DRHD:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_DRHD\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_DRHD:\n");
>              ret = acpi_parse_one_drhd(entry_header);
>              break;
>          case ACPI_DMAR_RMRR:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RMRR\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RMRR:\n");
>              ret = acpi_parse_one_rmrr(entry_header);
>              break;
>          case ACPI_DMAR_ATSR:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_ATSR\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_ATSR:\n");
>              ret = acpi_parse_one_atsr(entry_header);
>              break;
>          case ACPI_DMAR_RHSA:
> -            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RHSA\n");
> +            dprintk(XENLOG_INFO VTDPREFIX, "found
ACPI_DMAR_RHSA:\n");
>              ret = acpi_parse_one_rhsa(entry_header);
>              break;
>          default:
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Simon Horman

2009-Dec-03 02:55 UTC

head link

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

On Thu, Dec 03, 2009 at 10:08:59AM +0800, Han, Weidong
wrote:> Simon,
> 
> Thanks for your careful review.
> 
> Yes, the description is too simple. The patch mainly changes indention, but
also does some other cleanup as you pointed below. But Keir has already checked
in the patch.
No problem, the patch does seem fine despite my original comments.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Dec 2009 - [PATCH 1/2] VT-d: print message cleanup

[Xen-devel] [PATCH 1/2] VT-d: print message cleanup

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

RE: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup

Re: [Xen-devel] [PATCH 1/2] VT-d: print message cleanup