Re: [Xen-devel] [PATCH v2 2/5] iommu: introduce dom0-iommu option

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> Sent: Wednesday, August 1, 2018 7:04 PM
> 
> To select the iommu configuration used by Dom0. This option supersedes
> iommu=dom0-strict|dom0-passthrough.
> 
> No functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - New in this version.
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Cc: Brian Woods <brian.woods@xxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  docs/misc/xen-command-line.markdown         | 32 ++++++++++++++
>  xen/arch/x86/x86_64/mm.c                    |  3 +-
>  xen/drivers/passthrough/amd/iommu_init.c    |  2 +-
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |  4 +-
>  xen/drivers/passthrough/iommu.c             | 46 +++++++++++++++++----
>  xen/drivers/passthrough/vtd/iommu.c         | 16 +++----
>  xen/include/xen/iommu.h                     |  6 ++-
>  7 files changed, 88 insertions(+), 21 deletions(-)
> 
> diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> command-line.markdown
> index 65b4754418..a2a07cc6c8 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1150,12 +1150,18 @@ detection of systems known to misbehave
> upon accesses to that port.
> 
>  > `dom0-passthrough`
> 
> +> **WARNING: This command line option is deprecated, and superseded
> by
> +> _dom0-iommu=none_ - using both options in combination is
> undefined.**
> +

in patch description you said 'supersede'... is it better to say that
new dom0_iommu is favored if both options are specified than
saying 'undefined'?

>  > Default: `false`
> 
>  >> Control whether to disable DMA remapping for Dom0.
> 
>  > `dom0-strict`
> 
> +> **WARNING: This command line option is deprecated, and superseded
> by
> +> _dom0-iommu=strict_ - using both options in combination is
> undefined.**
> +
>  > Default: `false`
> 
>  >> Control whether to set up DMA remapping only for the memory Dom0
> actually
> @@ -1198,6 +1204,32 @@ detection of systems known to misbehave upon
> accesses to that port.
> 
>  >> Enable IOMMU debugging code (implies `verbose`).
> 
> +### dom0-iommu
> +> `= List of [ none | strict | relaxed ]`
> +
> +> Sub-options are of boolean kind and can be prefixed with `no-` to effect
> the
> +> inverse meaning.

not important comment, but doesn't "no-none" sound weird? :-)

> +
> +> `none`
> +
> +> Default: `false`
> +
> +>> Control whether to disable DMA remapping for Dom0.
> +
> +> `strict`
> +
> +> Default: `false`
> +
> +>> Control whether to set up DMA remapping only for the memory Dom0
> actually
> +>> got assigned.
> +
> +> `relaxed`
> +
> +> Default: `true`
> +
> +>> Controls whether to setup DMA remappings for all the host RAM
> except regions
> +>> in use by Xen.
> +
>  ### iommu\_dev\_iotlb\_timeout
>  > `= <integer>`
> 
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index cca4ae926e..84226b3326 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1426,7 +1426,8 @@ int memory_add(unsigned long spfn, unsigned
> long epfn, unsigned int pxm)
>      if ( ret )
>          goto destroy_m2p;
> 
> -    if ( iommu_enabled && !iommu_passthrough
> && !need_iommu(hardware_domain) )
> +    if ( iommu_enabled && !iommu_dom0_passthrough &&
> +         !need_iommu(hardware_domain) )
>      {
>          for ( i = spfn; i < epfn; i++ )
>              if ( iommu_map_page(hardware_domain, i, i,
> IOMMUF_readable|IOMMUF_writable) )
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c
> b/xen/drivers/passthrough/amd/iommu_init.c
> index 474992a75a..ad8c48be1c 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1062,7 +1062,7 @@ static void __init amd_iommu_init_cleanup(void)
>      radix_tree_destroy(&ivrs_maps, xfree);
> 
>      iommu_enabled = 0;
> -    iommu_passthrough = 0;
> +    iommu_dom0_passthrough = false;

ah, I see why "undefined" was used earlier. Both options control same
variable, thus the behavior is undefined when both are specified. If that
is the case, possibly clearer to say " This option *deprecates* iommu=
dom0-strict|dom0-passthrough" in patch description?

Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel