* [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default @ 2020-02-13 0:58 David Gibson 2020-02-13 0:58 ` [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later David Gibson ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: David Gibson @ 2020-02-13 0:58 UTC (permalink / raw) To: pair, qemu-devel, groug, clg Cc: mst, aik, paulus, mdroth, qemu-ppc, David Gibson Upcoming Secure VM support for pSeries machines introduces some complications for virtio, since the transfer buffers need to be explicitly shared so that the hypervisor can access them. While it's not strictly speaking dependent on it, the fact that virtio devices bypass normal platform IOMMU translation complicates the issue on the guest side. Since there are some significan downsides to bypassing the vIOMMU anyway, let's just disable that. There's already a flag to do this in virtio, just turn it on by default for forthcoming pseries machine types. Any opinions on whether dropping support for the older guest kernels is acceptable at this point? Changes since v1: * Added information on which guest kernel versions will no longer work with these changes * Use Michael Tsirkin's suggested better way of handling the machine type change David Gibson (2): spapr: Disable legacy virtio devices for pseries-5.0 and later spapr: Enable virtio iommu_platform=on by default hw/ppc/spapr.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later 2020-02-13 0:58 [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default David Gibson @ 2020-02-13 0:58 ` David Gibson 2020-02-13 14:34 ` Greg Kurz 2020-02-13 0:58 ` [PATCH v2 2/2] spapr: Enable virtio iommu_platform=on by default David Gibson 2020-02-13 11:46 ` [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio " Greg Kurz 2 siblings, 1 reply; 7+ messages in thread From: David Gibson @ 2020-02-13 0:58 UTC (permalink / raw) To: pair, qemu-devel, groug, clg Cc: mst, aik, paulus, mdroth, qemu-ppc, David Gibson PAPR specifies a kind of odd, paravirtualized PCI bus, which looks to the guess mostly like classic PCI, even if some of the individual devices on the bus are PCI Express. One consequence of that is that virtio-pci devices still default to being in transitional mode, though legacy mode is now disabled by default on current q35 x86 machine types. Legacy mode virtio devices aren't really necessary any more, and are causing some problems for future changes. Therefore, for the pseries-5.0 machine type (and onwards), switch to modern-only virtio-pci devices by default. This does mean we no longer support guest kernels prior to 4.0, unless they have modern virtio support backported (which some distro kernels like that in RHEL7 do). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index cb220fde45..6e1e467cc6 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -65,6 +65,7 @@ #include "hw/pci/pci.h" #include "hw/scsi/scsi.h" +#include "hw/virtio/virtio-pci.h" #include "hw/virtio/virtio-scsi.h" #include "hw/virtio/vhost-scsi-common.h" @@ -4567,7 +4568,14 @@ static void spapr_machine_latest_class_options(MachineClass *mc) */ static void spapr_machine_5_0_class_options(MachineClass *mc) { - /* Defaults for the latest behaviour inherited from the base class */ + /* Most defaults for the latest behaviour are inherited from the + * base class, but we need to override the (non ppc specific) + * default behaviour for virtio */ + static GlobalProperty compat[] = { + { TYPE_VIRTIO_PCI, "disable-legacy", "on", }, + }; + + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); } DEFINE_SPAPR_MACHINE(5_0, "5.0", true); @@ -4578,12 +4586,16 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true); static void spapr_machine_4_2_class_options(MachineClass *mc) { SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); + static GlobalProperty compat[] = { + { TYPE_VIRTIO_PCI, "disable-legacy", "auto" }, + }; spapr_machine_5_0_class_options(mc); compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF; smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF; mc->nvdimm_supported = false; + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); } DEFINE_SPAPR_MACHINE(4_2, "4.2", false); -- 2.24.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later 2020-02-13 0:58 ` [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later David Gibson @ 2020-02-13 14:34 ` Greg Kurz 2020-02-13 22:56 ` David Gibson 0 siblings, 1 reply; 7+ messages in thread From: Greg Kurz @ 2020-02-13 14:34 UTC (permalink / raw) To: David Gibson; +Cc: pair, mst, aik, qemu-devel, qemu-ppc, clg, mdroth, paulus On Thu, 13 Feb 2020 11:58:36 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > PAPR specifies a kind of odd, paravirtualized PCI bus, which looks to > the guess mostly like classic PCI, even if some of the individual > devices on the bus are PCI Express. One consequence of that is that > virtio-pci devices still default to being in transitional mode, though > legacy mode is now disabled by default on current q35 x86 machine > types. > > Legacy mode virtio devices aren't really necessary any more, and are > causing some problems for future changes. Therefore, for the > pseries-5.0 machine type (and onwards), switch to modern-only > virtio-pci devices by default. > > This does mean we no longer support guest kernels prior to 4.0, unless > they have modern virtio support backported (which some distro kernels > like that in RHEL7 do). > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index cb220fde45..6e1e467cc6 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -65,6 +65,7 @@ > > #include "hw/pci/pci.h" > #include "hw/scsi/scsi.h" > +#include "hw/virtio/virtio-pci.h" > #include "hw/virtio/virtio-scsi.h" > #include "hw/virtio/vhost-scsi-common.h" > > @@ -4567,7 +4568,14 @@ static void spapr_machine_latest_class_options(MachineClass *mc) > */ > static void spapr_machine_5_0_class_options(MachineClass *mc) > { > - /* Defaults for the latest behaviour inherited from the base class */ Hmm... and so it seems we still have to carry this around when we add a new machine version. At least, that's what I had to do when adding a dummy 5.1 machine. This is because it is the old machine type that calls the class_options() function of the new one, not the other way around. I was thinking about adapting Michael's patch. Instead of having a class_options() function that we only call for the latest machine type, we need a function that sets the default behaviour and call it for all machine types (which can still change the behaviour in their own class_options() function). Something like the following: ----------------------------------------------------------------------------- diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 828e2cc1359a..27e6f79ddc40 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4559,10 +4559,9 @@ static const TypeInfo spapr_machine_info = { }, }; -static void spapr_machine_latest_class_options(MachineClass *mc) +static void spapr_machine_default_class_options(MachineClass *mc) { - mc->alias = "pseries"; - mc->is_default = 1; + /* Override non ppc specific default behaviour here. */ } #define DEFINE_SPAPR_MACHINE(suffix, verstr, latest) \ @@ -4570,9 +4569,11 @@ static void spapr_machine_latest_class_options(MachineClass *mc) void *data) \ { \ MachineClass *mc = MACHINE_CLASS(oc); \ + spapr_machine_default_class_options(mc); \ spapr_machine_##suffix##_class_options(mc); \ if (latest) { \ - spapr_machine_latest_class_options(mc); \ + mc->alias = "pseries"; \ + mc->is_default = 1; \ } \ } \ static const TypeInfo spapr_machine_##suffix##_info = { \ @@ -4591,7 +4592,11 @@ static void spapr_machine_latest_class_options(MachineClass *mc) */ static void spapr_machine_5_0_class_options(MachineClass *mc) { - /* Defaults for the latest behaviour inherited from the base class */ + /* + * Most defaults for the latest behaviour are inherited from the + * base class. If a non ppc specific default behaviour needs to + * be overriden, do it in spapr_machine_latest_class_options(). + */ } DEFINE_SPAPR_MACHINE(5_0, "5.0", true); ----------------------------------------------------------------------------- With the above applied... > + /* Most defaults for the latest behaviour are inherited from the > + * base class, but we need to override the (non ppc specific) > + * default behaviour for virtio */ > + static GlobalProperty compat[] = { > + { TYPE_VIRTIO_PCI, "disable-legacy", "on", }, > + }; > + > + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); ... this change would just need to move to spapr_machine_default_class_options() and any need machine type would have it automatically. Makes sense ? > } > > DEFINE_SPAPR_MACHINE(5_0, "5.0", true); > @@ -4578,12 +4586,16 @@ DEFINE_SPAPR_MACHINE(5_0, "5.0", true); > static void spapr_machine_4_2_class_options(MachineClass *mc) > { > SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); > + static GlobalProperty compat[] = { > + { TYPE_VIRTIO_PCI, "disable-legacy", "auto" }, > + }; > > spapr_machine_5_0_class_options(mc); > compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len); > smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_OFF; > smc->default_caps.caps[SPAPR_CAP_FWNMI_MCE] = SPAPR_CAP_OFF; > mc->nvdimm_supported = false; > + compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); > } > > DEFINE_SPAPR_MACHINE(4_2, "4.2", false); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later 2020-02-13 14:34 ` Greg Kurz @ 2020-02-13 22:56 ` David Gibson 0 siblings, 0 replies; 7+ messages in thread From: David Gibson @ 2020-02-13 22:56 UTC (permalink / raw) To: Greg Kurz; +Cc: pair, mst, aik, qemu-devel, qemu-ppc, clg, mdroth, paulus [-- Attachment #1: Type: text/plain, Size: 3037 bytes --] On Thu, Feb 13, 2020 at 03:34:25PM +0100, Greg Kurz wrote: > On Thu, 13 Feb 2020 11:58:36 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > PAPR specifies a kind of odd, paravirtualized PCI bus, which looks to > > the guess mostly like classic PCI, even if some of the individual > > devices on the bus are PCI Express. One consequence of that is that > > virtio-pci devices still default to being in transitional mode, though > > legacy mode is now disabled by default on current q35 x86 machine > > types. > > > > Legacy mode virtio devices aren't really necessary any more, and are > > causing some problems for future changes. Therefore, for the > > pseries-5.0 machine type (and onwards), switch to modern-only > > virtio-pci devices by default. > > > > This does mean we no longer support guest kernels prior to 4.0, unless > > they have modern virtio support backported (which some distro kernels > > like that in RHEL7 do). > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index cb220fde45..6e1e467cc6 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -65,6 +65,7 @@ > > > > #include "hw/pci/pci.h" > > #include "hw/scsi/scsi.h" > > +#include "hw/virtio/virtio-pci.h" > > #include "hw/virtio/virtio-scsi.h" > > #include "hw/virtio/vhost-scsi-common.h" > > > > @@ -4567,7 +4568,14 @@ static void spapr_machine_latest_class_options(MachineClass *mc) > > */ > > static void spapr_machine_5_0_class_options(MachineClass *mc) > > { > > - /* Defaults for the latest behaviour inherited from the base class */ > > Hmm... and so it seems we still have to carry this around when we > add a new machine version. At least, that's what I had to do when > adding a dummy 5.1 machine. This is because it is the old machine > type that calls the class_options() function of the new one, not > the other way around. Uh.. no. It can just go in latest_class_options. I thought I'd already moved it there, but obviously not. I've fixed that up for the next spin. > I was thinking about adapting Michael's patch. Instead of having > a class_options() function that we only call for the latest > machine type, we need a function that sets the default behaviour > and call it for all machine types (which can still change the > behaviour in their own class_options() function). This will be confusing in a different way. It will reset the default options on each of the chained old machine types, which means anything set in the "default" options needs to be overriden in *all* old machine types that don't want it, not just the latest which doesn't want it. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] spapr: Enable virtio iommu_platform=on by default 2020-02-13 0:58 [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default David Gibson 2020-02-13 0:58 ` [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later David Gibson @ 2020-02-13 0:58 ` David Gibson 2020-02-13 11:46 ` [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio " Greg Kurz 2 siblings, 0 replies; 7+ messages in thread From: David Gibson @ 2020-02-13 0:58 UTC (permalink / raw) To: pair, qemu-devel, groug, clg Cc: mst, aik, paulus, mdroth, qemu-ppc, David Gibson Traditionally, virtio devices don't do DMA by the usual path on the guest platform. In particular they usually bypass any virtual IOMMU the guest has, using hypervisor magic to access untranslated guest physical addresses. There's now the optional iommu_platform flag which can tell virtio devices to use the platform's normal DMA path, including any IOMMUs. That flag was motiviated for the case of hardware virtio implementations, but there are other reasons to want it. Specifically, the fact that the virtio device doesn't use vIOMMU translation means that virtio devices are unsafe to pass to nested guests, or to use with VFIO userspace drivers inside the guest. This is particularly noticeable on the pseries platform which *always* has a guest-visible vIOMMU. Not using the normal DMA path also causes difficulties for the guest side driver when using the upcoming POWER Secure VMs (a.k.a. PEF). While it's theoretically possible to handle this on the guest side, it's really fiddly. Given the other problems with the non-translated virtio device, let's just enable vIOMMU translation for virtio devices by default in the pseries-5.0 (and later) machine types. This does mean the new machine type will no longer support guest kernels older than 4.8, unless they have support for the virtio IOMMU_PLATFORM flag backported (which some distro kernels like RHEL7 do). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 6e1e467cc6..d4f3dcdda5 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4573,6 +4573,7 @@ static void spapr_machine_5_0_class_options(MachineClass *mc) * default behaviour for virtio */ static GlobalProperty compat[] = { { TYPE_VIRTIO_PCI, "disable-legacy", "on", }, + { TYPE_VIRTIO_DEVICE, "iommu_platform", "on", }, }; compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); @@ -4588,6 +4589,7 @@ static void spapr_machine_4_2_class_options(MachineClass *mc) SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); static GlobalProperty compat[] = { { TYPE_VIRTIO_PCI, "disable-legacy", "auto" }, + { TYPE_VIRTIO_DEVICE, "iommu_platform", "off", }, }; spapr_machine_5_0_class_options(mc); -- 2.24.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default 2020-02-13 0:58 [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default David Gibson 2020-02-13 0:58 ` [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later David Gibson 2020-02-13 0:58 ` [PATCH v2 2/2] spapr: Enable virtio iommu_platform=on by default David Gibson @ 2020-02-13 11:46 ` Greg Kurz 2020-02-13 22:57 ` David Gibson 2 siblings, 1 reply; 7+ messages in thread From: Greg Kurz @ 2020-02-13 11:46 UTC (permalink / raw) To: David Gibson; +Cc: pair, mst, aik, qemu-devel, qemu-ppc, clg, mdroth, paulus On Thu, 13 Feb 2020 11:58:35 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > Upcoming Secure VM support for pSeries machines introduces some > complications for virtio, since the transfer buffers need to be > explicitly shared so that the hypervisor can access them. > > While it's not strictly speaking dependent on it, the fact that virtio > devices bypass normal platform IOMMU translation complicates the issue > on the guest side. Since there are some significan downsides to > bypassing the vIOMMU anyway, let's just disable that. > > There's already a flag to do this in virtio, just turn it on by > default for forthcoming pseries machine types. > > Any opinions on whether dropping support for the older guest kernels > is acceptable at this point? > As expected, this breaks compatibility with existing RHEL 6.10 guests. Each patch in this series requires an extra -global option to be specified on the command line in order to boot successfully. Patch 1: -global virtio-pci.disable-legacy=auto Patch 2: -global virtio-pci.iommu_platform=off As seen on the RH site [1], RHEL6 will reach "End of Maintenance Support or Maintenance Support 2 (Product retirement)" on November 30, 2020 and "End of Extended Life-cycle Support" on June 30, 2024. Not sure if it's okay to drop support for RHEL6 this soon. RHEL 7.7 guests seem to be unaffected. [1] https://access.redhat.com/support/policy/updates/errata/#Life_Cycle_Dates > Changes since v1: > * Added information on which guest kernel versions will no longer > work with these changes > * Use Michael Tsirkin's suggested better way of handling the machine > type change > > David Gibson (2): > spapr: Disable legacy virtio devices for pseries-5.0 and later > spapr: Enable virtio iommu_platform=on by default > > hw/ppc/spapr.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default 2020-02-13 11:46 ` [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio " Greg Kurz @ 2020-02-13 22:57 ` David Gibson 0 siblings, 0 replies; 7+ messages in thread From: David Gibson @ 2020-02-13 22:57 UTC (permalink / raw) To: Greg Kurz; +Cc: pair, mst, aik, qemu-devel, qemu-ppc, clg, mdroth, paulus [-- Attachment #1: Type: text/plain, Size: 2550 bytes --] On Thu, Feb 13, 2020 at 12:46:43PM +0100, Greg Kurz wrote: > On Thu, 13 Feb 2020 11:58:35 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > Upcoming Secure VM support for pSeries machines introduces some > > complications for virtio, since the transfer buffers need to be > > explicitly shared so that the hypervisor can access them. > > > > While it's not strictly speaking dependent on it, the fact that virtio > > devices bypass normal platform IOMMU translation complicates the issue > > on the guest side. Since there are some significan downsides to > > bypassing the vIOMMU anyway, let's just disable that. > > > > There's already a flag to do this in virtio, just turn it on by > > default for forthcoming pseries machine types. > > > > Any opinions on whether dropping support for the older guest kernels > > is acceptable at this point? > > > > As expected, this breaks compatibility with existing RHEL 6.10 guests. Each > patch in this series requires an extra -global option to be specified on > the command line in order to boot successfully. > > Patch 1: -global virtio-pci.disable-legacy=auto > Patch 2: -global virtio-pci.iommu_platform=off Right, or setting an older machine type. > As seen on the RH site [1], RHEL6 will reach "End of Maintenance Support > or Maintenance Support 2 (Product retirement)" on November 30, 2020 and > "End of Extended Life-cycle Support" on June 30, 2024. > > Not sure if it's okay to drop support for RHEL6 this soon. Hm, yeah. I'm happy enough to do this upstream, downstream will require some discussion. > RHEL 7.7 guests seem to be unaffected. Yeah, I already checked and RHEL7 has backported support for modern virtio and the iommu platform flag. > > [1] https://access.redhat.com/support/policy/updates/errata/#Life_Cycle_Dates > > > Changes since v1: > > * Added information on which guest kernel versions will no longer > > work with these changes > > * Use Michael Tsirkin's suggested better way of handling the machine > > type change > > > > David Gibson (2): > > spapr: Disable legacy virtio devices for pseries-5.0 and later > > spapr: Enable virtio iommu_platform=on by default > > > > hw/ppc/spapr.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-13 23:07 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-13 0:58 [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio by default David Gibson 2020-02-13 0:58 ` [PATCH v2 1/2] spapr: Disable legacy virtio devices for pseries-5.0 and later David Gibson 2020-02-13 14:34 ` Greg Kurz 2020-02-13 22:56 ` David Gibson 2020-02-13 0:58 ` [PATCH v2 2/2] spapr: Enable virtio iommu_platform=on by default David Gibson 2020-02-13 11:46 ` [PATCH v2 0/2] spapr: Use vIOMMU translation for virtio " Greg Kurz 2020-02-13 22:57 ` David Gibson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.