Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy

From: George Dunlap <george.dunlap@eu.citrix.com>
To: Jan Beulich <JBeulich@suse.com>, Tiejun Chen <tiejun.chen@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>,
	Ian Campbell <ian.campbell@citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
	Stefano Stabellini <stefano.stabellini@citrix.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Yang Zhang <yang.z.zhang@intel.com>,
	Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
Subject: Re: [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
Date: Fri, 10 Jul 2015 16:07:44 +0100	[thread overview]
Message-ID: <559FDFC0.7000609@eu.citrix.com> (raw)
In-Reply-To: <559FFA6C020000780008F998@mail.emea.novell.com>

On 07/10/2015 04:01 PM, Jan Beulich wrote:
>>>> On 10.07.15 at 15:26, <George.Dunlap@eu.citrix.com> wrote:
>> I also said that if we went with anything other than STRICT that we'd
>> need to check to make sure that the domain really was the hardware
>> domain before proceeding, in case the assumption that pdev->domain ==
>> hardware_domain ever changed.  (Perhaps with an ASSERT -- Jan, what do
>> you think?)
> 
> Yes, such an ASSERT() seems okay/desirable.
> 
>> Also, passing in RELAXED in locations where the flag is completely
>> ignored (such as when removing mappings) doesn't really make any
>> sense.
>>
>> On the whole I think it would be better if you removed the RELAXED
>> flag for both removals and for hardware domains.
> 
> But what would he pass instead? Or wait - iirc I had even suggested
> a way to do so by combining two arguments. Would need to go dig
> that out, because I think the idea got dropped without good reason.

No, I just meant to pass '0' for the flags (which would imply STRICT).

I was saying two things in the above paragraph:

1. For removal, there's no point in passing in anything other than '0'
for flags, since it's ignored.  Passing a non-0 value implies that the
flags will have some effect, which is misleading.

2. For places we know we're adding to hw domains, I think it makes most
sense also to pass in '0', to imply STRICT.

But if instead they insist on passing RELAXED, then please add an
ASSERT(pdev->domain == hw_domain) or something of the kind to
intel_iommu_add_device().  (If defaulting to STRICT, I don't think the
ASSERT is necessary anymore.)

 -George