Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.18 at 12:07, <roger.pau@xxxxxxxxxx> wrote:
> @@ -1198,6 +1204,23 @@ detection of systems known to misbehave upon accesses 
> to that port.
>  
>  >> Enable IOMMU debugging code (implies `verbose`).
>  
> +### dom0-iommu

This is now misplaced, as the file is (meant to be) alphabetically
sorted.

> +> `= List of [ none | strict | relaxed ]`
> +
> +* `none`: disables DMA remapping for Dom0.
> +
> +The following two options control how RAM regions are mapped in the iommu for
> +PV Dom0:
> +
> +* `strict`: sets up DMA remapping only for the memory Dom0 actually got
> +  assigned.

s/memory/RAM/ ?

> +* `relaxed`: sets DMA remapping for all the host RAM except regions in use by
> +  Xen. This is the default iommu behaviour.

Drop "iommu" here?

> +Note that all the above options are mutually exclusive. Specifying more than
> +one on the `dom0-iommu` command line will result in undefined behavior.

Isn't this more strict than it needs to be? "none", afaict, always takes
precedence if enabled. What color a bike shed is simply doesn't matter
when it doesn't exist.

> --- 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) )

This makes already clear that you need to better distinguish Dom0 and
hwdom here, but it's not immediately clear to me which direction the
changes should be made: Do you mean truly only Dom0 throughout
this patch, or hwdom? While the doc and command line option name can
perhaps left as is, internal variable names should not say Dom0 when
they don't mean Dom0. Otoh if you mean only Dom0, then the use of
hardware_domain above (and elsewhere) is now wrong.

Of course I won't demand (but even less so object to) you renaming
the other related variable that is affected here.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -22,6 +22,7 @@
>  #include <xsm/xsm.h>
>  
>  static int parse_iommu_param(const char *s);
> +static int parse_dom0_iommu_param(const char *s);

Please don't. Instead ...

> @@ -72,6 +71,10 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
>  bool_t __read_mostly iommu_debug;
>  bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>  
> +custom_param("dom0-iommu", parse_dom0_iommu_param);

... move this immediately after (with no intervening blank line)
parse_dom0_iommu_param()'s definition.

> +static int __init parse_dom0_iommu_param(const char *s)
> +{
> +    const char *ss;
> +    int rc = 0;
> +
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
> +
> +        if ( !strncmp(s, "none", ss - s) )
> +            iommu_dom0_passthrough = true;
> +        else if ( !strncmp(s, "strict", ss - s) )
> +            iommu_dom0_strict = true;
> +        else if ( !strncmp(s, "relaxed", ss - s) )
> +            iommu_dom0_strict = false;

Perhaps better just have one of the two, and make them truly
boolean? Or would that conflict with further patches / plans?

Jan



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