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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-06-23 at 17:57 +0800, Tiejun Chen wrote:
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a3e0e2e..638b350 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -655,6 +655,49 @@ assigned slave device.
>  
>  =back
>  
> +=item B<rdm="RDM_RESERVE_STRING">

Would RDM_RESERVATION_STRING more accurately describe this?

> +
> +(HVM/x86 only) Specifies the information about Reserved Device Memory (RDM),

Drop "the"

> +which is necessary to enable robust device passthrough. One example of RDM
> +is reported through ACPI Reserved Memory Region Reporting (RMRR) structure
> +on x86 platform.
> +
> +B<RDM_RESERVE_STRING> has the form C<[KEY=VALUE,KEY=VALUE,...> where:
> +
> +=over 4
> +
> +=item B<KEY=VALUE>
> +
> +Possible B<KEY>s are:
> +
> +=over 4
> +
> +=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"

> +"host" means all reserved device memory on this platform should be reserved
> +in this VM's guest address space space. This global RDM parameter allows

"space space"

> +user to specify reserved regions explicitly. And using "host" to include all
> +reserved regions reported on this platform which is good to handle hotplug
> +scenario.

I'm having trouble parsing this sentence, but I think you mean something
like:
        Using "host" includes all reserved regions reported on this
        platform, which is useful when doing hotplugging.

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

> +
> +"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?

> +
> +=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.

> +
> +=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...

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.

> +
> +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.

> +
> +Note this would override global B<rdm> option.
> +