Re: [v4][PATCH 11/19] tools: introduce some new parameters to set rdm policy

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

Thanks for your all corrections.


>> +=item B<type="STRING">
>> +
>> +Currently we just have two types:
>
> "Currently there are only two types". Although I would probably just say
> "Valid types are"

So let say "Currently there are only two valid types".

>
>> +"host" means all reserved device memory on this platform should be reserved

[snip]

>>   In the future this parameter may be further extended to allow
>> +specifying random regions, e.g. even those belonging to another platform as
>> +a preparation for live migration with passthrough devices.
>
> Lets document future stuff as it is implemented rather than leaving what
> is effectively a TODO in the face of the user.

Okay but I'm not very sure what's that format to introduce a TODO here. 
Maybe its just like this,

...
regions reported on this platform, which is useful when doing hotplugging.

TODO: in the future this parameter may be further extended to allow 
specifying arbitrary regions, e.g. even those belonging to another 
platform as a preparation for live migration with passthrough devices.

...

>
>> +
>> +"none" means we have nothing to do all reserved regions and ignore all policies,
>> +so guest work as before.
>
> This doesn't read right, but I'm not sure what you are trying to say so
> I can't suggest an alternative.
>
> How is type=none different from just not specifying rdm at all?

They're same behavior since "none" is our default option.

Just let me rephrase this,

"none" means we don't check any reserved regions and then all rdm 
policies would be ignored, so guest just work as before.

>
>> +
>> +=over 4
>
> Won't all these "=over 4"'s accumulate into a very deep indentation? I
> think you only need the first one (before the list) and the one before
> the nested list of types. In both cases you also need an "=back" at the
> end of the respective list to unwind the =over.

You're right so I also found this fault with `make docs` and I really 
should remove this here.

>
>> +
>> +=item B<reserve="STRING">
>> +
>> +Conflict may be detected when reserving reserved device memory in guest address
>> +space. "strict" means an unsolved conflict leads to immediate VM crash, while
>> +"relaxed" allows VM moving forward with a warning message thrown out. "relaxed"
>> +is default.
>
> I think I would say:
>
>          Specifies how to deal with conflicts discovered when reserving
>          reserved device memory in the guest address space. "strict"
>          means...
>

Nice and thanks.

> Having read all these docs I now know what all the options are, but I
> still don't really know what I should write. I think an example or two
> of real world usage would be helpful.

Here I picked some code fragments to help you understand this,

+        if(d_config->rdms[i].flag == LIBXL_RDM_RESERVE_FLAG_STRICT) {
+            LOG(ERROR, "RDM conflict at 0x%lx.\n", 
d_config->rdms[i].start);
+            goto out;
+        } else {
+            LOG(WARN, "Ignoring RDM conflict at 0x%lx.\n",
+                      d_config->rdms[i].start);
+
+            /*
+             * Then mask this INVALID to indicate we shouldn't expose this
+             * to hvmloader.
+             */
+            d_config->rdms[i].flag = LIBXL_RDM_RESERVE_FLAG_INVALID;
+        }
+    }
+
+    return 0;
+
+ out:
+    return ERROR_FAIL;


The above is just on tools side, and actually the similar also should 
happen on hypervisor side. Anyway, "strict" would make VM failed.

>
>> +
>> +Note this may be overridden by rdm_reserve option in PCI device configuration.
>> +
>>   =item B<pci=[ "PCI_SPEC_STRING", "PCI_SPEC_STRING", ... ]>
>>
>>   Specifies the host PCI devices to passthrough to this guest. Each B<PCI_SPEC_STRING>
>> @@ -717,6 +760,13 @@ dom0 without confirmation.  Please use with care.
>>   D0-D3hot power management states for the PCI device. False (0) by
>>   default.
>>
>> +=item B<rdm_reserv="STRING">
>> +
>> +(HVM/x86 only) This is same as reserve option above but just specific
>> +to a given device, and "strict" is default here.
>
> Rather than "above" (which is quite a large block of text) you should
> specifically mention the rdm option.
>

What about this?

(HVM/x86 only) This is same as reserve option inside the rdm option
but just specific to a given device, and "strict" is default here.

Thanks
Tiejun