Re: [v4][PATCH 04/19] xen/passthrough: extend hypercall to support rdm reservation policy

[George Dunlap <george.dunlap@eu.citrix.com>]

On 07/01/2015 11:47 AM, Chen, Tiejun wrote:
>>>  From my point of view, "NO" should be clear at certain point, right?
>>
>> Well, I'm afraid it's not.
>>
>> Looking through the entire series, it *appears* that "NO_RDM" is meant
>> to be passed for architectures like ARM DeviceTree, where it is known
>> that no RDM regions can exist.
>>
>> But it might also mean "I expect this device not to have any RDM
>> regions".  And it's certainly not immediately obvious what the effective
>> difference would be when I choose it -- what happens if I pass NO_RDM
>> for PCI systems?  How is it different than passing STRICT?
> 
> Currently NO_RDM is just used and specific to non-x86 inside Xen, not
> tools. So actually we don't pass this. If someone want to extend this
> usage in the future he really should take into account.

When you say "not tools", I take it to mean that you're not exposing
that option through the libxl interface?

tools/libxc/xc_domain.c:xc_assign_dt_device() most certainly does pass
it in, and that's the level I'm talking about.  Someone reviewing this
patch series needs to know, when xc or libxl set NO_RDM, what will be
the effect?  The fact that libxc *shouldn't* set NO_RDM for PCI devices
doesn't mean it won't happen.

Now looking at the end of the series and grepping for
"XEN_DOMCTL_DEV.*RDM", these values are *read and acted on* in exactly
two places:

xen/arch/x86/mm/p2m.c: The whole point of this series; if the p2m is
occupied already, and flag == RDM_STRICT, return an error; otherwise
ignore it.

xen/drivers/passthrough/device_tree.c: If flag != NO_RDM, return an error.

So the meaning of the flags is:
For pci devices:
 - RDM_RELAXED, NO_RDM: ignore conflicts in set_identity_p2m_entry()
 - RDM_STRICT: error on conflicts in set_identity_p2m_entry()
for dt devices:
 - Error if not NO_RDM

It doesn't look to me like the NO_RDM setting actually adds any semantic
meaning.

What I see in the list of references you gave is a request from the list
below is Julien saying this:

"I would also add a XEN_DOMCTL_DEV_NO_RDM that would be use for non-PCI
assignment."

It looks a bit like what you did is said, "Well Julien asked for a
NO_RDM setting, so here's a NO_RDM setting."  Which while perhaps
understandable, doesn't make the value have any more usefulness.

It seems to me that the real problem is that you had two values to begin
with, rather than actually having flags (as the name would imply).

This what I would suggest.  Make a single flag:

#define _XEN_DOMCTL_DEV_RDM_RELAXED     0
#define XEN_DOMCTL_DEV_RDM_RELAXED      (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED)

Then make the meaning of the flags as follows:
* for pci devices:
 - RDM_RELAXED flag SET: ignore conflicts in set_identity_p2m_entry()
 - RDM_RELAXED flag CLEAR: error on conflicts in set_identity_p2m_entry()
* for dt devices:
 - Ignore this flag entirely

If Julien really wants we could error on RDM_RELAXED being set; but I
don't think that's necessary.

 -George