[PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[Wei Liu]

This replicates a Xend behavior, see ec789523749 ("xend: Dis-allow
device assignment if PoD is enabled.").

This change is restricted to HVM guest, as only HVM is relevant in the
counterpart in Xend. We're late in release cycle so the change should
only do what's necessary. Probably we can revisit it if we need to do
the same thing for PV guest in the future.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
v2: fix comment
---
 tools/libxl/libxl_create.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e03bb55..61437de 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -706,6 +706,7 @@ static void initiate_domain_create(libxl__egc *egc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     uint32_t domid;
     int i, ret;
+    bool pod_enabled = false;
 
     /* convenience aliases */
     libxl_domain_config *const d_config = dcs->guest_config;
@@ -714,6 +715,25 @@ static void initiate_domain_create(libxl__egc *egc,
 
     domid = 0;
 
+    /* If target_memkb is smaller than max_memkb, the subsequent call
+     * to libxc when building HVM domain will enable PoD mode.
+     */
+    pod_enabled = (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) &&
+        (d_config->b_info.target_memkb < d_config->b_info.max_memkb);
+
+    /* We cannot have PoD and PCI device assignment at the same time
+     * for HVM guest. It was reported that IOMMU cannot work with PoD
+     * enabled because it needs to populated entire page table for
+     * guest. To stay on the safe side, we disable PCI device
+     * assignment when PoD is enabled.
+     */
+    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
+        d_config->num_pcidevs && pod_enabled) {
+        ret = ERROR_INVAL;
+        LOG(ERROR, "PCI device assignment for HVM guest failed due to PoD enabled");
+        goto error_out;
+    }
+
     ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
     if (ret) goto error_out;
 
-- 
1.7.10.4

Re: [PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[Ian Campbell]

On Mon, 2014-01-13 at 11:52 +0000, Wei Liu wrote:
> This replicates a Xend behavior, see ec789523749 ("xend: Dis-allow
> device assignment if PoD is enabled.").
> 
> This change is restricted to HVM guest, as only HVM is relevant in the
> counterpart in Xend. We're late in release cycle so the change should
> only do what's necessary. Probably we can revisit it if we need to do
> the same thing for PV guest in the future.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Release hat: The risk here is of a false positive detecting whether PoD
would be used and therefore refusing to start a domain. However Wei
directed me earlier on to the code in setup_guest which sets
XENMEMF_populate_on_demand and I believe it is using the same logic.

The benefit of this is that it will stop people starting a domain in an
invalid configuration -- but what is the downside here? Is it an
unhandled IOMMU fault or another host-fatal error? That would make the
argument for taking this patch pretty strong. On the other hand if the
failure were simply to kill this domain, that would be a less serious
issue and I'd be in two minds, mainly due to George not being here to
confirm that the pod_enabled logic is correct (although if he were here
I wouldn't be wrestling with this question at all ;-)).

I'm leaning towards taking this fix, but I'd really like to know what
the current failure case looks like.

Ian.

> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> v2: fix comment
> ---
>  tools/libxl/libxl_create.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index e03bb55..61437de 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -706,6 +706,7 @@ static void initiate_domain_create(libxl__egc *egc,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      uint32_t domid;
>      int i, ret;
> +    bool pod_enabled = false;
>  
>      /* convenience aliases */
>      libxl_domain_config *const d_config = dcs->guest_config;
> @@ -714,6 +715,25 @@ static void initiate_domain_create(libxl__egc *egc,
>  
>      domid = 0;
>  
> +    /* If target_memkb is smaller than max_memkb, the subsequent call
> +     * to libxc when building HVM domain will enable PoD mode.
> +     */
> +    pod_enabled = (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) &&
> +        (d_config->b_info.target_memkb < d_config->b_info.max_memkb);
> +
> +    /* We cannot have PoD and PCI device assignment at the same time
> +     * for HVM guest. It was reported that IOMMU cannot work with PoD
> +     * enabled because it needs to populated entire page table for
> +     * guest. To stay on the safe side, we disable PCI device
> +     * assignment when PoD is enabled.
> +     */
> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM &&
> +        d_config->num_pcidevs && pod_enabled) {
> +        ret = ERROR_INVAL;
> +        LOG(ERROR, "PCI device assignment for HVM guest failed due to PoD enabled");
> +        goto error_out;
> +    }
> +
>      ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
>      if (ret) goto error_out;
>  

Re: [PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[Andrew Cooper]

On 14/01/14 14:50, Ian Campbell wrote:
> On Mon, 2014-01-13 at 11:52 +0000, Wei Liu wrote:
>> This replicates a Xend behavior, see ec789523749 ("xend: Dis-allow
>> device assignment if PoD is enabled.").
>>
>> This change is restricted to HVM guest, as only HVM is relevant in the
>> counterpart in Xend. We're late in release cycle so the change should
>> only do what's necessary. Probably we can revisit it if we need to do
>> the same thing for PV guest in the future.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Release hat: The risk here is of a false positive detecting whether PoD
> would be used and therefore refusing to start a domain. However Wei
> directed me earlier on to the code in setup_guest which sets
> XENMEMF_populate_on_demand and I believe it is using the same logic.
>
> The benefit of this is that it will stop people starting a domain in an
> invalid configuration -- but what is the downside here? Is it an
> unhandled IOMMU fault or another host-fatal error? That would make the
> argument for taking this patch pretty strong. On the other hand if the
> failure were simply to kill this domain, that would be a less serious
> issue and I'd be in two minds, mainly due to George not being here to
> confirm that the pod_enabled logic is correct (although if he were here
> I wouldn't be wrestling with this question at all ;-)).
>
> I'm leaning towards taking this fix, but I'd really like to know what
> the current failure case looks like.
>
> Ian.

The answer is likely hardware specific.

An IOMMU fault (however handled by Xen) will result in a master abort on
the DMA transaction for the PCI device which has suffered the fault. 
That device can then do anything from continue blindly to issuing an NMI
IOCK/SERR which will likely be fatal to the entire server.

~Andrew

Re: [PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[Ian Campbell]

On Tue, 2014-01-14 at 14:54 +0000, Andrew Cooper wrote:
> On 14/01/14 14:50, Ian Campbell wrote:
> > On Mon, 2014-01-13 at 11:52 +0000, Wei Liu wrote:
> >> This replicates a Xend behavior, see ec789523749 ("xend: Dis-allow
> >> device assignment if PoD is enabled.").
> >>
> >> This change is restricted to HVM guest, as only HVM is relevant in the
> >> counterpart in Xend. We're late in release cycle so the change should
> >> only do what's necessary. Probably we can revisit it if we need to do
> >> the same thing for PV guest in the future.
> >>
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > Release hat: The risk here is of a false positive detecting whether PoD
> > would be used and therefore refusing to start a domain. However Wei
> > directed me earlier on to the code in setup_guest which sets
> > XENMEMF_populate_on_demand and I believe it is using the same logic.
> >
> > The benefit of this is that it will stop people starting a domain in an
> > invalid configuration -- but what is the downside here? Is it an
> > unhandled IOMMU fault or another host-fatal error? That would make the
> > argument for taking this patch pretty strong. On the other hand if the
> > failure were simply to kill this domain, that would be a less serious
> > issue and I'd be in two minds, mainly due to George not being here to
> > confirm that the pod_enabled logic is correct (although if he were here
> > I wouldn't be wrestling with this question at all ;-)).
> >
> > I'm leaning towards taking this fix, but I'd really like to know what
> > the current failure case looks like.
> >
> > Ian.
> 
> The answer is likely hardware specific.
> 
> An IOMMU fault (however handled by Xen) will result in a master abort on
> the DMA transaction for the PCI device which has suffered the fault. 
> That device can then do anything from continue blindly to issuing an NMI
> IOCK/SERR which will likely be fatal to the entire server.

Thanks, that tips me over into:
Release-ack: Ian Campbell

I applied, there was a reject against "libxl: Auto-assign NIC devids in
initiate_domain_create" which I fixed up.

Re: [PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[George Dunlap]

On 01/14/2014 02:54 PM, Andrew Cooper wrote:
> On 14/01/14 14:50, Ian Campbell wrote:
>> On Mon, 2014-01-13 at 11:52 +0000, Wei Liu wrote:
>>> This replicates a Xend behavior, see ec789523749 ("xend: Dis-allow
>>> device assignment if PoD is enabled.").
>>>
>>> This change is restricted to HVM guest, as only HVM is relevant in the
>>> counterpart in Xend. We're late in release cycle so the change should
>>> only do what's necessary. Probably we can revisit it if we need to do
>>> the same thing for PV guest in the future.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> Release hat: The risk here is of a false positive detecting whether PoD
>> would be used and therefore refusing to start a domain. However Wei
>> directed me earlier on to the code in setup_guest which sets
>> XENMEMF_populate_on_demand and I believe it is using the same logic.
>>
>> The benefit of this is that it will stop people starting a domain in an
>> invalid configuration -- but what is the downside here? Is it an
>> unhandled IOMMU fault or another host-fatal error? That would make the
>> argument for taking this patch pretty strong. On the other hand if the
>> failure were simply to kill this domain, that would be a less serious
>> issue and I'd be in two minds, mainly due to George not being here to
>> confirm that the pod_enabled logic is correct (although if he were here
>> I wouldn't be wrestling with this question at all ;-)).
>>
>> I'm leaning towards taking this fix, but I'd really like to know what
>> the current failure case looks like.
>>
>> Ian.
> The answer is likely hardware specific.
>
> An IOMMU fault (however handled by Xen) will result in a master abort on
> the DMA transaction for the PCI device which has suffered the fault.
> That device can then do anything from continue blindly to issuing an NMI
> IOCK/SERR which will likely be fatal to the entire server.

I thought we changed the behavior of Xen not to crash on SERR?  Or was I 
confused about that?

Since a VM should be able to craft a DMA pointing to invalid p2m space 
fairly easily, it seems like having the host crash in that scenario 
would basically remove half of the reason for having am IOMMU in the 
first place.

  -George

Re: [PATCH v2] libxl: disallow PCI device assignment for HVM guest when PoD is enabled

[Andrew Cooper]

On 20/01/14 14:04, George Dunlap wrote:
> On 01/14/2014 02:54 PM, Andrew Cooper wrote:
>> On 14/01/14 14:50, Ian Campbell wrote:
>>> On Mon, 2014-01-13 at 11:52 +0000, Wei Liu wrote:
>>>> This replicates a Xend behavior, see ec789523749 ("xend: Dis-allow
>>>> device assignment if PoD is enabled.").
>>>>
>>>> This change is restricted to HVM guest, as only HVM is relevant in the
>>>> counterpart in Xend. We're late in release cycle so the change should
>>>> only do what's necessary. Probably we can revisit it if we need to do
>>>> the same thing for PV guest in the future.
>>>>
>>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> Release hat: The risk here is of a false positive detecting whether PoD
>>> would be used and therefore refusing to start a domain. However Wei
>>> directed me earlier on to the code in setup_guest which sets
>>> XENMEMF_populate_on_demand and I believe it is using the same logic.
>>>
>>> The benefit of this is that it will stop people starting a domain in an
>>> invalid configuration -- but what is the downside here? Is it an
>>> unhandled IOMMU fault or another host-fatal error? That would make the
>>> argument for taking this patch pretty strong. On the other hand if the
>>> failure were simply to kill this domain, that would be a less serious
>>> issue and I'd be in two minds, mainly due to George not being here to
>>> confirm that the pod_enabled logic is correct (although if he were here
>>> I wouldn't be wrestling with this question at all ;-)).
>>>
>>> I'm leaning towards taking this fix, but I'd really like to know what
>>> the current failure case looks like.
>>>
>>> Ian.
>> The answer is likely hardware specific.
>>
>> An IOMMU fault (however handled by Xen) will result in a master abort on
>> the DMA transaction for the PCI device which has suffered the fault.
>> That device can then do anything from continue blindly to issuing an NMI
>> IOCK/SERR which will likely be fatal to the entire server.
>
> I thought we changed the behavior of Xen not to crash on SERR?  Or was
> I confused about that?
>
> Since a VM should be able to craft a DMA pointing to invalid p2m space
> fairly easily, it seems like having the host crash in that scenario
> would basically remove half of the reason for having am IOMMU in the
> first place.
>
>  -George

The behaviour of IOCK/SERR NMIs depends on the "nmi=" command line
option, which for a non-debug build is "redirect to dom0", and in a
debug build is fatal.  Dom0's behaviour is normally to say "huh - my
virtual environment looks fine", which makes it the option quite useless.

IOCK/SERR NMIs can be out-of-band, possibly via the BMC on a server
class motherboard, and per XSA-59, possibly not caught by the IOMMU.

~Andrew