Hotplugging PCI devices to Xen PV guests is only possible if the
libxl_domain_build_info struct has the e820_host field enabled when the
guest is created. By default it is disabled but libxl will automatically
enable e820_host if the config contains one or more PCI devices, in which
case hotplugging additional PCI devices later works.

According to xl.cfg(5) man page it is safe to unconditionally enable the
PV-only e820_host setting. Furthermore xen.git commits 414979ba85 and
f92337d949, which introduce the setting with a default of disabled, claim
the setting can be enabled or even removed "once the auto-ballooning of
guests with PCI devices works". Those commits are from May 2011 so I
think it is safe to say the issues have been resolved in the meantime.
Regardless, we should avoid exposing a Xen setting in libvirt that could
be removed later.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

See related thread on the ML

https://www.redhat.com/archives/libvir-list/2020-April/msg00376.html


 src/libxl/libxl_conf.c                        | 5 +++++
 tests/libxlxml2domconfigdata/basic-pv.json    | 3 ++-
 tests/libxlxml2domconfigdata/multiple-ip.json | 3 ++-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index be5fc505fe..f6280157fb 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -288,6 +288,11 @@ libxlMakeDomBuildInfo(virDomainDefPtr def,
 #endif
     } else {
         libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
+        /*
+         * e820_host is a PV-only setting and according to xl.cfg(5) it
+         * should be safe to unconditionally enable it.
+         */
+        libxl_defbool_set(&b_info->u.pv.e820_host, true);
     }
 
     b_info->max_vcpus = virDomainDefGetVcpusMax(def);
diff --git a/tests/libxlxml2domconfigdata/basic-pv.json b/tests/libxlxml2domconfigdata/basic-pv.json
index b71c3b0f49..2cb95feff8 100644
--- a/tests/libxlxml2domconfigdata/basic-pv.json
+++ b/tests/libxlxml2domconfigdata/basic-pv.json
@@ -19,7 +19,8 @@
 
         },
         "type.pv": {
-            "bootloader": "pygrub"
+            "bootloader": "pygrub",
+            "e820_host": "True"
         },
         "arch_arm": {
 
diff --git a/tests/libxlxml2domconfigdata/multiple-ip.json b/tests/libxlxml2domconfigdata/multiple-ip.json
index 2db98b82f6..01bb169cdc 100644
--- a/tests/libxlxml2domconfigdata/multiple-ip.json
+++ b/tests/libxlxml2domconfigdata/multiple-ip.json
@@ -19,7 +19,8 @@
 
         },
         "type.pv": {
-            "bootloader": "pygrub"
+            "bootloader": "pygrub",
+            "e820_host": "True"
         },
         "arch_arm": {
 
-- 
2.26.0

On Wed, Apr 08, 2020 at 02:29:16PM -0600, Jim Fehlig wrote:
> Hotplugging PCI devices to Xen PV guests is only possible if the
> libxl_domain_build_info struct has the e820_host field enabled when the
> guest is created. By default it is disabled but libxl will automatically
> enable e820_host if the config contains one or more PCI devices, in which
> case hotplugging additional PCI devices later works.
> 
> According to xl.cfg(5) man page it is safe to unconditionally enable the
> PV-only e820_host setting. Furthermore xen.git commits 414979ba85 and
> f92337d949, which introduce the setting with a default of disabled, claim
> the setting can be enabled or even removed "once the auto-ballooning of
> guests with PCI devices works". Those commits are from May 2011 so I
> think it is safe to say the issues have been resolved in the meantime.
> Regardless, we should avoid exposing a Xen setting in libvirt that could
> be removed later.

Does this have any implications for live migration compatibility if you
silently enable this for all guests ?

In QEMU/KVM if you did this, it would be considered an ABI change and
could break live migration of a guest launched on old libvirt, to a
host running new libvirt.

> 
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> 
> See related thread on the ML
> 
> https://www.redhat.com/archives/libvir-list/2020-April/msg00376.html
> 
> 
>  src/libxl/libxl_conf.c                        | 5 +++++
>  tests/libxlxml2domconfigdata/basic-pv.json    | 3 ++-
>  tests/libxlxml2domconfigdata/multiple-ip.json | 3 ++-
>  3 files changed, 9 insertions(+), 2 deletions(-)

*if* the live migration compatibility is not a problem, then

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 4/9/20 7:14 AM, Daniel P. Berrangé wrote:
> On Wed, Apr 08, 2020 at 02:29:16PM -0600, Jim Fehlig wrote:
>> Hotplugging PCI devices to Xen PV guests is only possible if the
>> libxl_domain_build_info struct has the e820_host field enabled when the
>> guest is created. By default it is disabled but libxl will automatically
>> enable e820_host if the config contains one or more PCI devices, in which
>> case hotplugging additional PCI devices later works.
>>
>> According to xl.cfg(5) man page it is safe to unconditionally enable the
>> PV-only e820_host setting. Furthermore xen.git commits 414979ba85 and
>> f92337d949, which introduce the setting with a default of disabled, claim
>> the setting can be enabled or even removed "once the auto-ballooning of
>> guests with PCI devices works". Those commits are from May 2011 so I
>> think it is safe to say the issues have been resolved in the meantime.
>> Regardless, we should avoid exposing a Xen setting in libvirt that could
>> be removed later.
> 
> Does this have any implications for live migration compatibility if you
> silently enable this for all guests ?

Oh, right. Thanks for the reminder! I'll have to check but I suspect it will.

> In QEMU/KVM if you did this, it would be considered an ABI change and
> could break live migration of a guest launched on old libvirt, to a
> host running new libvirt.

Nod. Do you have any suggestions on how to model this setting in libvirt? I 
proposed adding a hypervisor feature for Xen in this thread

https://www.redhat.com/archives/libvir-list/2020-April/msg00376.html

rational being that for PV guests the hypervisor serves as the BIOS and provides 
the facility to report the memory map to the OS. I couldn't really think of a 
good fit for it within the <os> element and its children.

Regards,
Jim

On Thu, Apr 09, 2020 at 02:52:30PM -0600, Jim Fehlig wrote:
> On 4/9/20 7:14 AM, Daniel P. Berrangé wrote:
> > On Wed, Apr 08, 2020 at 02:29:16PM -0600, Jim Fehlig wrote:
> > > Hotplugging PCI devices to Xen PV guests is only possible if the
> > > libxl_domain_build_info struct has the e820_host field enabled when the
> > > guest is created. By default it is disabled but libxl will automatically

This isn't fully true: xl will do that, not libxl. Which means under
libvirt it will always be disabled.

> > > enable e820_host if the config contains one or more PCI devices, in which
> > > case hotplugging additional PCI devices later works.
> > > 
> > > According to xl.cfg(5) man page it is safe to unconditionally enable the
> > > PV-only e820_host setting. Furthermore xen.git commits 414979ba85 and
> > > f92337d949, which introduce the setting with a default of disabled, claim
> > > the setting can be enabled or even removed "once the auto-ballooning of
> > > guests with PCI devices works". Those commits are from May 2011 so I
> > > think it is safe to say the issues have been resolved in the meantime.
> > > Regardless, we should avoid exposing a Xen setting in libvirt that could
> > > be removed later.
> > 
> > Does this have any implications for live migration compatibility if you
> > silently enable this for all guests ?
> 
> Oh, right. Thanks for the reminder! I'll have to check but I suspect it will.

Can a VM with PCI device be live migrated? If not, it shouldn't be an
issue if you enable it only for PCI-having domains (similarly as xl
does). Or patch it in libxl, see this discussion:
http://xen.markmail.org/thread/awcswnywzei4s65e

> > In QEMU/KVM if you did this, it would be considered an ABI change and
> > could break live migration of a guest launched on old libvirt, to a
> > host running new libvirt.
> 
> Nod. Do you have any suggestions on how to model this setting in libvirt? I
> proposed adding a hypervisor feature for Xen in this thread
> 
> https://www.redhat.com/archives/libvir-list/2020-April/msg00376.html
> 
> rational being that for PV guests the hypervisor serves as the BIOS and
> provides the facility to report the memory map to the OS. I couldn't really
> think of a good fit for it within the <os> element and its children.

FWIW, in Qubes we have a patches adding e820_host setting here:
https://github.com/QubesOS/qubes-core-libvirt/
(patches 8-11)
Not submitted before, exactly to avoid adding temporary options. But
since 8+ years later it is still there, I think it's safe to assume it
will be there for some more. Or at least it's worth to unbreak some
configurations in the meantime.

I'll rebase them on master and post here.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:
> FWIW, in Qubes we have a patches adding e820_host setting here:
> https://github.com/QubesOS/qubes-core-libvirt/
> (patches 8-11)
> Not submitted before, exactly to avoid adding temporary options. But
> since 8+ years later it is still there, I think it's safe to assume it
> will be there for some more. Or at least it's worth to unbreak some
> configurations in the meantime.

BTW, the other piece of the puzzle for PCI passthrough starting with xen 4.13 is 
the xl.cfg 'passthrough' option. See xen commit babde47a3fe. Without that 
enabled, hotplugging a PCI device to an HVM or PV domain is not possible, with 
or without e820_host. AFAICT there are no patches in your queue for the 
'passthrough' option. Perhaps you haven't rebased to xen 4.13. Do you have any 
thoughts on modeling that option?

My thoughts are adding another xen hypervisor feature (similar to e820_host) or 
map it to the new IOMMU device in libvirt [1]. I suppose it depends on the need 
to support the {sync,share}_pt values vs a simple enable/disable. If enabling 
and disabling passthrough is enough, I'd lean towards another xen hypervisor 
feature. Otherwise we'll need to explore the IOMMU device?

Regards,
Jim

[1] https://libvirt.org/formatdomain.html#elementsIommu

On Tue, Apr 14, 2020 at 03:56:47PM -0600, Jim Fehlig wrote:
> On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:
> > FWIW, in Qubes we have a patches adding e820_host setting here:
> > https://github.com/QubesOS/qubes-core-libvirt/
> > (patches 8-11)
> > Not submitted before, exactly to avoid adding temporary options. But
> > since 8+ years later it is still there, I think it's safe to assume it
> > will be there for some more. Or at least it's worth to unbreak some
> > configurations in the meantime.
> 
> BTW, the other piece of the puzzle for PCI passthrough starting with xen
> 4.13 is the xl.cfg 'passthrough' option. See xen commit babde47a3fe. Without
> that enabled, hotplugging a PCI device to an HVM or PV domain is not
> possible, with or without e820_host. AFAICT there are no patches in your
> queue for the 'passthrough' option. Perhaps you haven't rebased to xen 4.13.

TBH, I don't do PCI hotplugging.

> Do you have any thoughts on modeling that option?
>
> My thoughts are adding another xen hypervisor feature (similar to e820_host)
> or map it to the new IOMMU device in libvirt [1]. I suppose it depends on
> the need to support the {sync,share}_pt values vs a simple enable/disable.
> If enabling and disabling passthrough is enough, I'd lean towards another
> xen hypervisor feature. Otherwise we'll need to explore the IOMMU device?

Isn't IOMMU device about allowing the guest to control (v)IOMMU? I've seen
some discussion about it in context of Xen, so IMO we should not use it
now for something else.
What about adding <controller type='pci'/> ?
I'm not sure about {sync,share}_pt, but probably could be squeezed there
if needed.

> Regards,
> Jim
> 
> [1] https://libvirt.org/formatdomain.html#elementsIommu

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

On 4/14/20 4:07 PM, Marek Marczykowski-Górecki wrote:
> On Tue, Apr 14, 2020 at 03:56:47PM -0600, Jim Fehlig wrote:
>> On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:
>>> FWIW, in Qubes we have a patches adding e820_host setting here:
>>> https://github.com/QubesOS/qubes-core-libvirt/
>>> (patches 8-11)
>>> Not submitted before, exactly to avoid adding temporary options. But
>>> since 8+ years later it is still there, I think it's safe to assume it
>>> will be there for some more. Or at least it's worth to unbreak some
>>> configurations in the meantime.
>>
>> BTW, the other piece of the puzzle for PCI passthrough starting with xen
>> 4.13 is the xl.cfg 'passthrough' option. See xen commit babde47a3fe. Without
>> that enabled, hotplugging a PCI device to an HVM or PV domain is not
>> possible, with or without e820_host. AFAICT there are no patches in your
>> queue for the 'passthrough' option. Perhaps you haven't rebased to xen 4.13.
> 
> TBH, I don't do PCI hotplugging.

Lucky you :-).

>> Do you have any thoughts on modeling that option?
>>
>> My thoughts are adding another xen hypervisor feature (similar to e820_host)
>> or map it to the new IOMMU device in libvirt [1]. I suppose it depends on
>> the need to support the {sync,share}_pt values vs a simple enable/disable.
>> If enabling and disabling passthrough is enough, I'd lean towards another
>> xen hypervisor feature. Otherwise we'll need to explore the IOMMU device?
> 
> Isn't IOMMU device about allowing the guest to control (v)IOMMU? I've seen
> some discussion about it in context of Xen, so IMO we should not use it
> now for something else.
> What about adding <controller type='pci'/> ?
> I'm not sure about {sync,share}_pt, but probably could be squeezed there
> if needed.

I suppose adding a PCI controller is one way of configuring the guest to later 
hotplug PCI devices, although I'm not sure if (sync,share}_pt could be 
considered attributes of a PCI controller. Also, xen has no notion of a PCI 
controller that I'm aware of, so it would only exist in libvirt. It seems like a 
heavy-weight approach for a setting that essentially enables or disables PCI 
passthrough.

Daniel, do you have any ideas about modeling the xl.cfg(5) 'passthrough' option? 
For convenience, see 'passthrough' under "Other Options"

https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html#Other-Options

Regards,
Jim

On 4/13/20 1:17 PM, Marek Marczykowski-Górecki wrote:
> On Thu, Apr 09, 2020 at 02:52:30PM -0600, Jim Fehlig wrote:
>> On 4/9/20 7:14 AM, Daniel P. Berrangé wrote:
>>> On Wed, Apr 08, 2020 at 02:29:16PM -0600, Jim Fehlig wrote:
>>>> Hotplugging PCI devices to Xen PV guests is only possible if the
>>>> libxl_domain_build_info struct has the e820_host field enabled when the
>>>> guest is created. By default it is disabled but libxl will automatically
> 
> This isn't fully true: xl will do that, not libxl. Which means under
> libvirt it will always be disabled.

Ah, thanks for pointing that out! I should have learned by now to take a closer 
look. The default of some settings is handled in xl, whereas others in libxl.

>>>> enable e820_host if the config contains one or more PCI devices, in which
>>>> case hotplugging additional PCI devices later works.
>>>>
>>>> According to xl.cfg(5) man page it is safe to unconditionally enable the
>>>> PV-only e820_host setting. Furthermore xen.git commits 414979ba85 and
>>>> f92337d949, which introduce the setting with a default of disabled, claim
>>>> the setting can be enabled or even removed "once the auto-ballooning of
>>>> guests with PCI devices works". Those commits are from May 2011 so I
>>>> think it is safe to say the issues have been resolved in the meantime.
>>>> Regardless, we should avoid exposing a Xen setting in libvirt that could
>>>> be removed later.
>>>
>>> Does this have any implications for live migration compatibility if you
>>> silently enable this for all guests ?
>>
>> Oh, right. Thanks for the reminder! I'll have to check but I suspect it will.
> 
> Can a VM with PCI device be live migrated? If not, it shouldn't be an
> issue if you enable it only for PCI-having domains (similarly as xl
> does).

This patch enables e820_host regardless if the domain has a PCI device. The 
concern is migrating a domain running on a host without this patch (e820_host 
disabled) to a host with this patch, where magically e820_host becomes enabled 
when the domain config is created. I suspect the OS running inside that domain 
would not be happy.

> Or patch it in libxl, see this discussion:
> http://xen.markmail.org/thread/awcswnywzei4s65e

I would prefer if such odd settings could be handled internally in libxl :-).

>>> In QEMU/KVM if you did this, it would be considered an ABI change and
>>> could break live migration of a guest launched on old libvirt, to a
>>> host running new libvirt.
>>
>> Nod. Do you have any suggestions on how to model this setting in libvirt? I
>> proposed adding a hypervisor feature for Xen in this thread
>>
>> https://www.redhat.com/archives/libvir-list/2020-April/msg00376.html
>>
>> rational being that for PV guests the hypervisor serves as the BIOS and
>> provides the facility to report the memory map to the OS. I couldn't really
>> think of a good fit for it within the <os> element and its children.
> 
> FWIW, in Qubes we have a patches adding e820_host setting here:
> https://github.com/QubesOS/qubes-core-libvirt/
> (patches 8-11)
> Not submitted before, exactly to avoid adding temporary options. But
> since 8+ years later it is still there, I think it's safe to assume it
> will be there for some more. Or at least it's worth to unbreak some
> configurations in the meantime.

Agreed.

> I'll rebase them on master and post here.

Thanks!

Regards,
Jim