Re: [Xen-devel] [PATCH 1 of 3] IOMMU: Add command line param to disable sharing of IOMMU and hap tables

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.12 at 11:44, Tim Deegan <tim@xxxxxxx> wrote:
> At 09:17 +0000 on 22 Mar (1332407837), Jan Beulich wrote:
>>  >>> On 21.03.12 at 20:22, Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx> 
>> wrote:
>> > The default is 1, and the command line parameter sets to 1. Disabling may 
> be
>> > desired if the host will contain VMs that do paging, sharing or mem 
>> > access, 
> 
>> > and
>> > won't be doing passthrough. These two features are mutually exclusive for 
>> > AMD
>> > processors.
>> > 
>> > Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>> > 
>> > diff -r d7e417afcbe4 -r 642c0e6a01c2 xen/drivers/passthrough/iommu.c
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -87,6 +87,8 @@ static void __init parse_iommu_param(cha
>> >              iommu_dom0_strict = 1;
>> >          else if ( !strcmp(s, "sharept") )
>> >              iommu_hap_pt_share = 1;
>> > +        else if ( !strcmp(s, "no-sharept") )
>> > +            iommu_hap_pt_share = 0;
>> 
>> Taking the description above into consideration - why not simply
>> replace the pointless enabling option with the disabling one?
> 
> Let's just fix the whole function to handle 'no-' the way the main
> parser does:

Good idea, but ...

> -----
> 
> IOMMU: clean up handling of 'foo' and 'no-foo' command-line options
> 
> Signed-off-by: Tim Deegan <tim@xxxxxxx>
> 
> diff -r 8180cb3895af xen/drivers/passthrough/iommu.c
> --- a/xen/drivers/passthrough/iommu.c Thu Mar 22 10:26:45 2012 +0000
> +++ b/xen/drivers/passthrough/iommu.c Thu Mar 22 10:38:48 2012 +0000
> @@ -57,36 +57,41 @@ DEFINE_PER_CPU(bool_t, iommu_dont_flush_
>  static void __init parse_iommu_param(char *s)
>  {
>      char *ss;
> +    int val;
>  
>      do {
> +        val = !!strncmp(s, "no-", 3);
> +        if ( !val )
> +            s += 3;
> +
>          ss = strchr(s, ',');
>          if ( ss )
>              *ss = '\0';
>  
>          if ( !parse_bool(s) )
> -            iommu_enabled = 0;
> +            iommu_enabled = val;

... this one must remain unchanged, and the handling of the "no-"
prefix should come afterwards.

Jan

>          else if ( !strcmp(s, "force") || !strcmp(s, "required") )
> -            force_iommu = 1;
> +            force_iommu = val;
>          else if ( !strcmp(s, "workaround_bios_bug") )
>              iommu_workaround_bios_bug = 1;
>          else if ( !strcmp(s, "verbose") )
> -            iommu_verbose = 1;
> -        else if ( !strcmp(s, "no-snoop") )
> -            iommu_snoop = 0;
> -        else if ( !strcmp(s, "no-qinval") )
> -            iommu_qinval = 0;
> -        else if ( !strcmp(s, "no-intremap") )
> -            iommu_intremap = 0;
> +            iommu_verbose = val;
> +        else if ( !strcmp(s, "snoop") )
> +            iommu_snoop = val;
> +        else if ( !strcmp(s, "qinval") )
> +            iommu_qinval = val;
> +        else if ( !strcmp(s, "intremap") )
> +            iommu_intremap = val;
>          else if ( !strcmp(s, "debug") )
> -            iommu_debug = 1;
> +            iommu_debug = val;
>          else if ( !strcmp(s, "amd-iommu-perdev-intremap") )
> -            amd_iommu_perdev_intremap = 1;
> +            amd_iommu_perdev_intremap = val;
>          else if ( !strcmp(s, "dom0-passthrough") )
> -            iommu_passthrough = 1;
> +            iommu_passthrough = val;
>          else if ( !strcmp(s, "dom0-strict") )
> -            iommu_dom0_strict = 1;
> +            iommu_dom0_strict = val;
>          else if ( !strcmp(s, "sharept") )
> -            iommu_hap_pt_share = 1;
> +            iommu_hap_pt_share = val;
>  
>          s = ss + 1;
>      } while ( ss );




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel