Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
On 07/14/2015 12:45 PM, Jan Beulich wrote: On 14.07.15 at 13:30, wrote: >> On 07/14/2015 11:53 AM, Chen, Tiejun wrote: The way this sort of thing is defined in the rest of domctl.h is like this: #define _XEN_DOMCTL_CDF_hvm_guest 0 #define XEN_DOMCTL_CDF_hvm_guest (1U<<_XEN_DOMCTL_CDF_hvm_guest) So the above should be #define _XEN_DOMCTL_DEV_RDM_RELAXED 0 #define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED) And then your check in iommu_do_pci_domctl() would look like if (flag & ~XEN_DOMCTL_DEV_RDM_RELAXED) And if we end up adding any extra flags, we just | them into the above conditional, as is done in, for example, the XEN_DOMCTL_createdomain case in xen/common/domctl.c:do_domctl(). >>> >>> Seems Jan didn't like this way IIRC, so I hope Jan also can have a look >>> at this beforehand :) >> >> I think Jan thought that the MASK value you defined wasn't meant to be a >> single flag, but for all the flags; i.e., that if we added flags in bits >> 1 and 2, that MASK would become 0x7 rather than 0x1. And I agree that >> there's not much point to having such a mask defined in the public header. >> >> But what I'm doing above is making explicit what you have already; i.e., >> you just set XEN_DOMCTL_DEV_RDM_RELAXED to '1'; the reader has to sort >> of infer that the reason '1' is chosen is that it's setting bit 0. >> Doing it the way I suggest makes it more clear that this is meant to be >> a bitfield, and '0' has been allocated. >> >> Please correct me if I'm wrong, Jan. > > Indeed my primary objection was to what you describe. That said, > I'm not too happy with what you propose now either. Not only do > I view this (bit-pos,bit-mask) tuple as redundant unless one actually > needs both in certain places (which doesn't seem to be the case > here), but the naming also conflicts with the C standard (reserving > identifiers starting with underscore and an upper case letter). Just > like said in various other cases - we've got many examples of such > violations already, but I'd prefer not to make the situation worse. > > IOW I'd prefer to go with just > > #define XEN_DOMCTL_DEV_RDM_RELAXED 1 Very well. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
>>> On 14.07.15 at 13:30, wrote: > On 07/14/2015 11:53 AM, Chen, Tiejun wrote: >>> The way this sort of thing is defined in the rest of domctl.h is like >>> this: >>> >>> #define _XEN_DOMCTL_CDF_hvm_guest 0 >>> #define XEN_DOMCTL_CDF_hvm_guest (1U<<_XEN_DOMCTL_CDF_hvm_guest) >>> >>> So the above should be >>> >>> #define _XEN_DOMCTL_DEV_RDM_RELAXED 0 >>> #define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED) >>> >>> And then your check in iommu_do_pci_domctl() would look like >>> >>> if (flag & ~XEN_DOMCTL_DEV_RDM_RELAXED) >>> >>> And if we end up adding any extra flags, we just | them into the above >>> conditional, as is done in, for example, the XEN_DOMCTL_createdomain >>> case in xen/common/domctl.c:do_domctl(). >>> >> >> Seems Jan didn't like this way IIRC, so I hope Jan also can have a look >> at this beforehand :) > > I think Jan thought that the MASK value you defined wasn't meant to be a > single flag, but for all the flags; i.e., that if we added flags in bits > 1 and 2, that MASK would become 0x7 rather than 0x1. And I agree that > there's not much point to having such a mask defined in the public header. > > But what I'm doing above is making explicit what you have already; i.e., > you just set XEN_DOMCTL_DEV_RDM_RELAXED to '1'; the reader has to sort > of infer that the reason '1' is chosen is that it's setting bit 0. > Doing it the way I suggest makes it more clear that this is meant to be > a bitfield, and '0' has been allocated. > > Please correct me if I'm wrong, Jan. Indeed my primary objection was to what you describe. That said, I'm not too happy with what you propose now either. Not only do I view this (bit-pos,bit-mask) tuple as redundant unless one actually needs both in certain places (which doesn't seem to be the case here), but the naming also conflicts with the C standard (reserving identifiers starting with underscore and an upper case letter). Just like said in various other cases - we've got many examples of such violations already, but I'd prefer not to make the situation worse. IOW I'd prefer to go with just #define XEN_DOMCTL_DEV_RDM_RELAXED 1 Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
On 07/14/2015 11:53 AM, Chen, Tiejun wrote: >> The way this sort of thing is defined in the rest of domctl.h is like >> this: >> >> #define _XEN_DOMCTL_CDF_hvm_guest 0 >> #define XEN_DOMCTL_CDF_hvm_guest (1U<<_XEN_DOMCTL_CDF_hvm_guest) >> >> So the above should be >> >> #define _XEN_DOMCTL_DEV_RDM_RELAXED 0 >> #define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED) >> >> And then your check in iommu_do_pci_domctl() would look like >> >> if (flag & ~XEN_DOMCTL_DEV_RDM_RELAXED) >> >> And if we end up adding any extra flags, we just | them into the above >> conditional, as is done in, for example, the XEN_DOMCTL_createdomain >> case in xen/common/domctl.c:do_domctl(). >> > > Seems Jan didn't like this way IIRC, so I hope Jan also can have a look > at this beforehand :) I think Jan thought that the MASK value you defined wasn't meant to be a single flag, but for all the flags; i.e., that if we added flags in bits 1 and 2, that MASK would become 0x7 rather than 0x1. And I agree that there's not much point to having such a mask defined in the public header. But what I'm doing above is making explicit what you have already; i.e., you just set XEN_DOMCTL_DEV_RDM_RELAXED to '1'; the reader has to sort of infer that the reason '1' is chosen is that it's setting bit 0. Doing it the way I suggest makes it more clear that this is meant to be a bitfield, and '0' has been allocated. Please correct me if I'm wrong, Jan. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
The way this sort of thing is defined in the rest of domctl.h is like this: #define _XEN_DOMCTL_CDF_hvm_guest 0 #define XEN_DOMCTL_CDF_hvm_guest (1U<<_XEN_DOMCTL_CDF_hvm_guest) So the above should be #define _XEN_DOMCTL_DEV_RDM_RELAXED 0 #define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED) And then your check in iommu_do_pci_domctl() would look like if (flag & ~XEN_DOMCTL_DEV_RDM_RELAXED) And if we end up adding any extra flags, we just | them into the above conditional, as is done in, for example, the XEN_DOMCTL_createdomain case in xen/common/domctl.c:do_domctl(). Seems Jan didn't like this way IIRC, so I hope Jan also can have a look at this beforehand :) Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
On 07/13/2015 07:47 AM, Chen, Tiejun wrote:
>> Thanks for this; a few more comments...
>>
>
> Thanks for your time.
>
>>> @@ -1577,9 +1578,15 @@ int iommu_do_pci_domctl(
>>> seg = machine_sbdf >> 16;
>>> bus = PCI_BUS(machine_sbdf);
>>> devfn = PCI_DEVFN2(machine_sbdf);
>>> +flag = domctl->u.assign_device.flag;
>>> +if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
>>
>> This is not a blocker, but a stylistic comment: I would have inverted
>> the bitmask here, as that's conceptually what you're checking. I
>> won't make this a blocker for going in.
>
> What about this?
>
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 6e23fc6..17a4206 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1579,7 +1579,7 @@ int iommu_do_pci_domctl(
> bus = PCI_BUS(machine_sbdf);
> devfn = PCI_DEVFN2(machine_sbdf);
> flag = domctl->u.assign_device.flag;
> -if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
> +if ( flag & ~XEN_DOMCTL_DEV_RDM_MASK )
> {
> ret = -EINVAL;
> break;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index bca25c9..07549a4 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -480,6 +480,7 @@ struct xen_domctl_assign_device {
> } u;
> /* IN */
> #define XEN_DOMCTL_DEV_RDM_RELAXED 1
> +#define XEN_DOMCTL_DEV_RDM_MASK 0x1
The way this sort of thing is defined in the rest of domctl.h is like this:
#define _XEN_DOMCTL_CDF_hvm_guest 0
#define XEN_DOMCTL_CDF_hvm_guest (1U<<_XEN_DOMCTL_CDF_hvm_guest)
So the above should be
#define _XEN_DOMCTL_DEV_RDM_RELAXED 0
#define XEN_DOMCTL_DEV_RDM_RELAXED (1U<<_XEN_DOMCTL_DEV_RDM_RELAXED)
And then your check in iommu_do_pci_domctl() would look like
if (flag & ~XEN_DOMCTL_DEV_RDM_RELAXED)
And if we end up adding any extra flags, we just | them into the above
conditional, as is done in, for example, the XEN_DOMCTL_createdomain
case in xen/common/domctl.c:do_domctl().
Thanks,
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
>>> On 13.07.15 at 08:47, wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -480,6 +480,7 @@ struct xen_domctl_assign_device {
> } u;
> /* IN */
> #define XEN_DOMCTL_DEV_RDM_RELAXED 1
> +#define XEN_DOMCTL_DEV_RDM_MASK 0x1
As said before - I dislike this mask being made part of the public
interface, albeit it being a domctl thing makes it a minor issue.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
Thanks for this; a few more comments...
Thanks for your time.
@@ -1577,9 +1578,15 @@ int iommu_do_pci_domctl(
seg = machine_sbdf >> 16;
bus = PCI_BUS(machine_sbdf);
devfn = PCI_DEVFN2(machine_sbdf);
+flag = domctl->u.assign_device.flag;
+if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
This is not a blocker, but a stylistic comment: I would have inverted
the bitmask here, as that's conceptually what you're checking. I
won't make this a blocker for going in.
What about this?
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 6e23fc6..17a4206 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1579,7 +1579,7 @@ int iommu_do_pci_domctl(
bus = PCI_BUS(machine_sbdf);
devfn = PCI_DEVFN2(machine_sbdf);
flag = domctl->u.assign_device.flag;
-if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
+if ( flag & ~XEN_DOMCTL_DEV_RDM_MASK )
{
ret = -EINVAL;
break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index bca25c9..07549a4 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -480,6 +480,7 @@ struct xen_domctl_assign_device {
} u;
/* IN */
#define XEN_DOMCTL_DEV_RDM_RELAXED 1
+#define XEN_DOMCTL_DEV_RDM_MASK 0x1
uint32_t flag; /* flag of assigned device */
};
typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;
@@ -1898,7 +1899,14 @@ static int intel_iommu_add_device(u8 devfn, struct
pci_dev *pdev)
PCI_BUS(bdf) == pdev->bus &&
PCI_DEVFN2(bdf) == devfn )
{
-ret = rmrr_identity_mapping(pdev->domain, 1, rmrr);
+/*
+ * Here means we're add a device to the hardware domain
+ * so actually RMRR is always reserved on e820 so either
+ * of flag is fine for hardware domain and here we'd like
+ * to pass XEN_DOMCTL_DEV_RDM_RELAXED.
+ */
Sorry I didn't give feedback on your comment before. I don't find it
clear enough; I'd suggest something like this:
"iommu_add_device() is only called for the hardware domain (see
xen/drivers/passthrough/pci.c:pci_add_device()). Since RMRRs are
always reserved in the e820 map for the hardware domain, there
shouldn't be a conflict."
Loos good and thanks.
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?)
Sounds reasonable.
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.
Just as I said in another email I agreed your STRICT way.
Thanks
Tiejun
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
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.) I agree and also looks Jan didn't oppose this STRICT way by setting "0" directly, so lets do this. Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
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 don't drop this directly. http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01877.html I went there with one optional way you provided that I just need to a brief comment. And I also had reply at that moment. http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg02101.html Thanks Tiejun ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
On 07/10/2015 04:01 PM, Jan Beulich wrote: On 10.07.15 at 15:26, 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
>>> On 10.07.15 at 15:26, 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. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy
On Thu, Jul 9, 2015 at 6:33 AM, Tiejun Chen wrote:
> This patch extends the existing hypercall to support rdm reservation policy.
> We return error or just throw out a warning message depending on whether
> the policy is "strict" or "relaxed" when reserving RDM regions in pfn space.
> Note in some special cases, e.g. add a device to hwdomain, and remove a
> device from user domain, 'relaxed' is fine enough since this is always safe
> to hwdomain.
>
> CC: Tim Deegan
> CC: Keir Fraser
> CC: Jan Beulich
> CC: Andrew Cooper
> CC: Suravee Suthikulpanit
> CC: Aravind Gopalakrishnan
> CC: Ian Campbell
> CC: Stefano Stabellini
> CC: Yang Zhang
> CC: Kevin Tian
> Signed-off-by: Tiejun Chen
> ---
> v6 ~ v7:
>
> * Nothing is changed.
>
> v5:
>
> * Just leave one bit XEN_DOMCTL_DEV_RDM_RELAXED as our flag, so
> "0" means "strict" and "1" means "relaxed".
Thanks for this; a few more comments...
> @@ -1577,9 +1578,15 @@ int iommu_do_pci_domctl(
> seg = machine_sbdf >> 16;
> bus = PCI_BUS(machine_sbdf);
> devfn = PCI_DEVFN2(machine_sbdf);
> +flag = domctl->u.assign_device.flag;
> +if ( flag > XEN_DOMCTL_DEV_RDM_RELAXED )
This is not a blocker, but a stylistic comment: I would have inverted
the bitmask here, as that's conceptually what you're checking. I
won't make this a blocker for going in.
> @@ -1898,7 +1899,14 @@ static int intel_iommu_add_device(u8 devfn, struct
> pci_dev *pdev)
> PCI_BUS(bdf) == pdev->bus &&
> PCI_DEVFN2(bdf) == devfn )
> {
> -ret = rmrr_identity_mapping(pdev->domain, 1, rmrr);
> +/*
> + * Here means we're add a device to the hardware domain
> + * so actually RMRR is always reserved on e820 so either
> + * of flag is fine for hardware domain and here we'd like
> + * to pass XEN_DOMCTL_DEV_RDM_RELAXED.
> + */
Sorry I didn't give feedback on your comment before. I don't find it
clear enough; I'd suggest something like this:
"iommu_add_device() is only called for the hardware domain (see
xen/drivers/passthrough/pci.c:pci_add_device()). Since RMRRs are
always reserved in the e820 map for the hardware domain, there
shouldn't be a conflict."
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?)
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.
-George
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
