Re: [v5][PATCH 10/16] tools: introduce some new parameters to set rdm policy

["Chen, Tiejun" <tiejun.chen@intel.com>]

>> Global RDM parameter, "type", allows user to specify reserved regions
>> explicitly, e.g. using 'host' to include all reserved regions reported
>
> As I understand this feature, I don't think the name `type' is right.
> This is a method for handling or overriding/ignoring the RDM problem.
>
> Perhaps `handling=' or `strategy=' ?

Okay, I prefer to use 'strategy='.

>
> And `none' seems quite an innocuous name for such a dangerous
> setting.  How about `ignore' ?
>
>> 'strict/relaxed' policy decides how to handle conflict when reserving RDM
>> regions in pfn space. If conflict exists, 'strict' means an immediate error
>> so VM can't keep running, while 'relaxed' allows moving forward with a
>> warning message thrown out.
>>
>> Default per-device RDM policy is 'strict', while default global RDM policy
>> is 'relaxed'. And the per-device policy would override the global policy like
>> others.
>
> I don't think these two sentences can both be true.  If the per-device
> policy overrides the global policy, then the per-device policy must
> have at least three values: `strict', `relaxed', and `not set'
> (implicitly using global policy).

Yeah. The per-device policy is first set as the global policy. This 
behavior is same as other pci option. Please see the last patch,

@@ -1937,6 +1945,11 @@ skip_vfb:
              pcidev->power_mgmt = pci_power_mgmt;
              pcidev->permissive = pci_permissive;
              pcidev->seize = pci_seize;
+            /*
+             * Like other pci option, the per-device policy always follows
+             * the global policy by default.
+             */
+            pcidev->rdm_reserve = b_info->u.hvm.rdm.reserve;
              if (!xlu_pci_parse_bdf(config, pcidev, buf))
                  d_config->num_pcidevs++;
          }

>
> Perhaps I have misunderstood.
>
>> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
>> index a3e0e2e..a2dc343 100644
>> --- a/docs/man/xl.cfg.pod.5
>> +++ b/docs/man/xl.cfg.pod.5
>> @@ -655,6 +655,83 @@ assigned slave device.
> ...
>> +=item B<type="STRING">
>
> The documentation on this is rather vague about what the default is.
> But you do say:
>
>> +In this conflict case,
>> +
>> +#1. If the type options is set with "none",
>> +
>> +rdm = "type=none,reserve=strict" or rdm = "type=none,reserve=relaxed"
>> +
>> +mean we don't handle any conflict just to make VM keep running as before.
>> +Note this is our default behavior.
>
> This suggests that the default is "do the dangerous thing".  That
> doesn't seem right.

As I discussed with Campbell we'd like not to expose "none" in xl level 
since this is equivalent to that case we don't set anything.

>
>> +#2. If the type options is set with "host",
>> +
>> +rdm = "type=host,reserve=strict" or rdm = "type=host,reserve=relaxed"
> ...
>> +"relaxed" allows a VM to be created to keep running with a warning message
>> +thrown out. But this may crash this VM if this device accesses RDM. For example,
>> +Windows IGD GFX driver always access these regions so this lead to a blue screen
>> +to crash VM in such a case.
>
> The difference between
>     type=host,reserve=relaxed
> and
>     type=none
> is simply whether a warning is printed ?
>

"none" means we have a chance to work as before since not all devices 
own RDM. But as I said above this is same as !rdm.

Thanks
Tiejun