* [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
@ 2020-04-09 12:10 Leif Lindholm
2020-04-09 13:07 ` Ard Biesheuvel
2020-04-09 13:56 ` Laszlo Ersek
0 siblings, 2 replies; 7+ messages in thread
From: Leif Lindholm @ 2020-04-09 12:10 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Laszlo Ersek, Leendert van Doorn
From: Leendert van Doorn <leendert@microsoft.com>
Enable conditional support for NVMe storage in ArmVirtQemu/
QemVirtQemuKernel in order to simplify booting/installing operating
systems that don't support virtio.
[Conditionalised driver inclusion]
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 +++++++
ArmVirtPkg/ArmVirtQemuKernel.dsc | 8 ++++++++
3 files changed, 23 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 8c77fc46427b..6f93032ac064 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -28,6 +28,7 @@ [Defines]
# -D FLAG=VALUE
#
DEFINE TTY_TERMINAL = FALSE
+ DEFINE NVME_ENABLE = FALSE
DEFINE SECURE_BOOT_ENABLE = FALSE
DEFINE TPM2_ENABLE = FALSE
DEFINE TPM2_CONFIG_ENABLE = FALSE
@@ -447,6 +448,13 @@ [Components.common]
MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
+ #
+ # NVME Driver
+ #
+!if $(NVME_ENABLE) == TRUE
+ MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
+!endif
+
#
# SMBIOS Support
#
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index aaba0b1c8840..45f0dd65be33 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -128,6 +128,13 @@ [FV.FvMain]
INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
+ #
+ # NVME Driver
+ #
+!if $(NVME_ENABLE) == TRUE
+ INF MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
+!endif
+
#
# SMBIOS Support
#
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index 4d82a77213ec..5dd4b1cf29f4 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -28,6 +28,7 @@ [Defines]
# -D FLAG=VALUE
#
DEFINE TTY_TERMINAL = FALSE
+ DEFINE NVME_ENABLE = FALSE
DEFINE SECURE_BOOT_ENABLE = FALSE
#
@@ -382,6 +383,13 @@ [Components.common]
MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
+ #
+ # NVME Driver
+ #
+!if $(NVME_ENABLE) == TRUE
+ MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
+!endif
+
#
# SMBIOS Support
#
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
2020-04-09 12:10 [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu* Leif Lindholm
@ 2020-04-09 13:07 ` Ard Biesheuvel
2020-04-09 14:31 ` Leif Lindholm
2020-04-09 13:56 ` Laszlo Ersek
1 sibling, 1 reply; 7+ messages in thread
From: Ard Biesheuvel @ 2020-04-09 13:07 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Laszlo Ersek, Leendert van Doorn
On 4/9/20 2:10 PM, Leif Lindholm wrote:
> From: Leendert van Doorn <leendert@microsoft.com>
>
> Enable conditional support for NVMe storage in ArmVirtQemu/
> QemVirtQemuKernel in order to simplify booting/installing operating
> systems that don't support virtio.
>
> [Conditionalised driver inclusion]
Why?
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 +++++++
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 8 ++++++++
> 3 files changed, 23 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 8c77fc46427b..6f93032ac064 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -28,6 +28,7 @@ [Defines]
> # -D FLAG=VALUE
> #
> DEFINE TTY_TERMINAL = FALSE
> + DEFINE NVME_ENABLE = FALSE
> DEFINE SECURE_BOOT_ENABLE = FALSE
> DEFINE TPM2_ENABLE = FALSE
> DEFINE TPM2_CONFIG_ENABLE = FALSE
> @@ -447,6 +448,13 @@ [Components.common]
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
> + #
> + # NVME Driver
> + #
> +!if $(NVME_ENABLE) == TRUE
> + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> +!endif
> +
> #
> # SMBIOS Support
> #
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index aaba0b1c8840..45f0dd65be33 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -128,6 +128,13 @@ [FV.FvMain]
> INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
> + #
> + # NVME Driver
> + #
> +!if $(NVME_ENABLE) == TRUE
> + INF MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> +!endif
> +
> #
> # SMBIOS Support
> #
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 4d82a77213ec..5dd4b1cf29f4 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -28,6 +28,7 @@ [Defines]
> # -D FLAG=VALUE
> #
> DEFINE TTY_TERMINAL = FALSE
> + DEFINE NVME_ENABLE = FALSE
> DEFINE SECURE_BOOT_ENABLE = FALSE
>
> #
> @@ -382,6 +383,13 @@ [Components.common]
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
> + #
> + # NVME Driver
> + #
> +!if $(NVME_ENABLE) == TRUE
> + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> +!endif
> +
> #
> # SMBIOS Support
> #
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
2020-04-09 12:10 [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu* Leif Lindholm
2020-04-09 13:07 ` Ard Biesheuvel
@ 2020-04-09 13:56 ` Laszlo Ersek
2020-04-09 14:53 ` Leif Lindholm
1 sibling, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2020-04-09 13:56 UTC (permalink / raw)
To: Leif Lindholm, devel; +Cc: Ard Biesheuvel, Leendert van Doorn
On 04/09/20 14:10, Leif Lindholm wrote:
> From: Leendert van Doorn <leendert@microsoft.com>
>
> Enable conditional support for NVMe storage in ArmVirtQemu/
> QemVirtQemuKernel in order to simplify booting/installing operating
> systems that don't support virtio.
(1) We also have UsbMassStorageDxe in ArmVirtQemu*, which can drive
QEMU's "usb-storage" device model.
In case that device+driver combo has been tested too, and
unsuccessfully, then I suggest explicitly stating in the commit message:
"don't support virtio or usb-storage".
If UsbMassStorageDxe has *not* been tested, then I agree it should not
be mentioned here at all.
For more details -- in case someone is interested in testing
"usb-storage" --, please refer to commit f9c59fa44ae2
("OvmfPkg/QemuBootOrderLib: recognize "usb-storage" devices in XHCI
ports", 2017-09-22). That commit message includes both QEMU command line
and libvirt domain XML examples.
>
> [Conditionalised driver inclusion]
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 +++++++
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 8 ++++++++
> 3 files changed, 23 insertions(+)
(2) I realize I'm being arbitrary, with regard to what driver should be
conditional and what should always be there. But in OvmfPkg,
NvmExpressDxe is included unconditionally, and I think that should work
here too.
Of course, if you *want* to make NvmExpressDxe excludable from the
ArmVirtQemu* platforms, then I'm fine with that.
So, I'm proposing (1) and (2) only as food for thought; dependent on
your stance on them, I'm OK with this patch:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks,
Laszlo
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 8c77fc46427b..6f93032ac064 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -28,6 +28,7 @@ [Defines]
> # -D FLAG=VALUE
> #
> DEFINE TTY_TERMINAL = FALSE
> + DEFINE NVME_ENABLE = FALSE
> DEFINE SECURE_BOOT_ENABLE = FALSE
> DEFINE TPM2_ENABLE = FALSE
> DEFINE TPM2_CONFIG_ENABLE = FALSE
> @@ -447,6 +448,13 @@ [Components.common]
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
> + #
> + # NVME Driver
> + #
> +!if $(NVME_ENABLE) == TRUE
> + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> +!endif
> +
> #
> # SMBIOS Support
> #
> diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> index aaba0b1c8840..45f0dd65be33 100644
> --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> @@ -128,6 +128,13 @@ [FV.FvMain]
> INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
> + #
> + # NVME Driver
> + #
> +!if $(NVME_ENABLE) == TRUE
> + INF MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> +!endif
> +
> #
> # SMBIOS Support
> #
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index 4d82a77213ec..5dd4b1cf29f4 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -28,6 +28,7 @@ [Defines]
> # -D FLAG=VALUE
> #
> DEFINE TTY_TERMINAL = FALSE
> + DEFINE NVME_ENABLE = FALSE
> DEFINE SECURE_BOOT_ENABLE = FALSE
>
> #
> @@ -382,6 +383,13 @@ [Components.common]
> MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
>
> + #
> + # NVME Driver
> + #
> +!if $(NVME_ENABLE) == TRUE
> + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> +!endif
> +
> #
> # SMBIOS Support
> #
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
2020-04-09 13:07 ` Ard Biesheuvel
@ 2020-04-09 14:31 ` Leif Lindholm
2020-04-09 14:36 ` Ard Biesheuvel
0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2020-04-09 14:31 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Laszlo Ersek, Leendert van Doorn
On Thu, Apr 09, 2020 at 15:07:30 +0200, Ard Biesheuvel wrote:
> On 4/9/20 2:10 PM, Leif Lindholm wrote:
> > From: Leendert van Doorn <leendert@microsoft.com>
> >
> > Enable conditional support for NVMe storage in ArmVirtQemu/
> > QemVirtQemuKernel in order to simplify booting/installing operating
> > systems that don't support virtio.
> >
> > [Conditionalised driver inclusion]
>
> Why?
Well, it adds size and complexity for a target most frequently used to
model a pure virtual environment?
Size difference isn't huge though:
FVMAIN [99%Full] 5337920 total, 5337864 used, 56 free
FVMAIN_COMPACT [39%Full] 2093056 total, 829392 used, 1263664 free
vs.
FVMAIN [99%Full] 5300992 total, 5300936 used, 56 free
FVMAIN_COMPACT [38%Full] 2093056 total, 804128 used, 1288928 free
If that's not an issue, I'm happy to drop it and my bracketing.
/
Leif
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> > ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
> > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 +++++++
> > ArmVirtPkg/ArmVirtQemuKernel.dsc | 8 ++++++++
> > 3 files changed, 23 insertions(+)
> >
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 8c77fc46427b..6f93032ac064 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -28,6 +28,7 @@ [Defines]
> > # -D FLAG=VALUE
> > #
> > DEFINE TTY_TERMINAL = FALSE
> > + DEFINE NVME_ENABLE = FALSE
> > DEFINE SECURE_BOOT_ENABLE = FALSE
> > DEFINE TPM2_ENABLE = FALSE
> > DEFINE TPM2_CONFIG_ENABLE = FALSE
> > @@ -447,6 +448,13 @@ [Components.common]
> > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > + #
> > + # NVME Driver
> > + #
> > +!if $(NVME_ENABLE) == TRUE
> > + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > +!endif
> > +
> > #
> > # SMBIOS Support
> > #
> > diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > index aaba0b1c8840..45f0dd65be33 100644
> > --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > @@ -128,6 +128,13 @@ [FV.FvMain]
> > INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > + #
> > + # NVME Driver
> > + #
> > +!if $(NVME_ENABLE) == TRUE
> > + INF MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > +!endif
> > +
> > #
> > # SMBIOS Support
> > #
> > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > index 4d82a77213ec..5dd4b1cf29f4 100644
> > --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > @@ -28,6 +28,7 @@ [Defines]
> > # -D FLAG=VALUE
> > #
> > DEFINE TTY_TERMINAL = FALSE
> > + DEFINE NVME_ENABLE = FALSE
> > DEFINE SECURE_BOOT_ENABLE = FALSE
> > #
> > @@ -382,6 +383,13 @@ [Components.common]
> > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > + #
> > + # NVME Driver
> > + #
> > +!if $(NVME_ENABLE) == TRUE
> > + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > +!endif
> > +
> > #
> > # SMBIOS Support
> > #
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
2020-04-09 14:31 ` Leif Lindholm
@ 2020-04-09 14:36 ` Ard Biesheuvel
0 siblings, 0 replies; 7+ messages in thread
From: Ard Biesheuvel @ 2020-04-09 14:36 UTC (permalink / raw)
To: Leif Lindholm; +Cc: devel, Laszlo Ersek, Leendert van Doorn
On 4/9/20 4:31 PM, Leif Lindholm wrote:
> On Thu, Apr 09, 2020 at 15:07:30 +0200, Ard Biesheuvel wrote:
>> On 4/9/20 2:10 PM, Leif Lindholm wrote:
>>> From: Leendert van Doorn <leendert@microsoft.com>
>>>
>>> Enable conditional support for NVMe storage in ArmVirtQemu/
>>> QemVirtQemuKernel in order to simplify booting/installing operating
>>> systems that don't support virtio.
>>>
>>> [Conditionalised driver inclusion]
>>
>> Why?
>
> Well, it adds size and complexity for a target most frequently used to
> model a pure virtual environment?
>
> Size difference isn't huge though:
> FVMAIN [99%Full] 5337920 total, 5337864 used, 56 free
> FVMAIN_COMPACT [39%Full] 2093056 total, 829392 used, 1263664 free
> vs.
> FVMAIN [99%Full] 5300992 total, 5300936 used, 56 free
> FVMAIN_COMPACT [38%Full] 2093056 total, 804128 used, 1288928 free
>
> If that's not an issue, I'm happy to drop it and my bracketing.
>
I agree that in general, driver stacks like network or TPM should be
opt-in, but given the reason you are adding this [compatibility with
other OSes], I'd prefer to have this one always built in.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
2020-04-09 13:56 ` Laszlo Ersek
@ 2020-04-09 14:53 ` Leif Lindholm
2020-04-09 17:43 ` Leif Lindholm
0 siblings, 1 reply; 7+ messages in thread
From: Leif Lindholm @ 2020-04-09 14:53 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: devel, Ard Biesheuvel, Leendert van Doorn
On Thu, Apr 09, 2020 at 15:56:20 +0200, Laszlo Ersek wrote:
> On 04/09/20 14:10, Leif Lindholm wrote:
> > From: Leendert van Doorn <leendert@microsoft.com>
> >
> > Enable conditional support for NVMe storage in ArmVirtQemu/
> > QemVirtQemuKernel in order to simplify booting/installing operating
> > systems that don't support virtio.
>
> (1) We also have UsbMassStorageDxe in ArmVirtQemu*, which can drive
> QEMU's "usb-storage" device model.
>
> In case that device+driver combo has been tested too, and
> unsuccessfully, then I suggest explicitly stating in the commit message:
> "don't support virtio or usb-storage".
Well, in this particular case we were more interested in the (much)
lower protocol overhead. But that is a very good point in general.
> If UsbMassStorageDxe has *not* been tested, then I agree it should not
> be mentioned here at all.
I have not, but I'll chuck it on the pile of stuff to test at some
point.
> For more details -- in case someone is interested in testing
> "usb-storage" --, please refer to commit f9c59fa44ae2
> ("OvmfPkg/QemuBootOrderLib: recognize "usb-storage" devices in XHCI
> ports", 2017-09-22). That commit message includes both QEMU command line
> and libvirt domain XML examples.
Thanks!
> > [Conditionalised driver inclusion]
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> > ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
> > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 +++++++
> > ArmVirtPkg/ArmVirtQemuKernel.dsc | 8 ++++++++
> > 3 files changed, 23 insertions(+)
>
> (2) I realize I'm being arbitrary, with regard to what driver should be
> conditional and what should always be there. But in OvmfPkg,
> NvmExpressDxe is included unconditionally, and I think that should work
> here too.
>
> Of course, if you *want* to make NvmExpressDxe excludable from the
> ArmVirtQemu* platforms, then I'm fine with that.
I don't. I just succumbed to premature optimization.
Do you want to see a v2, or do I drop the bits before submission?
Regards,
Leif
> So, I'm proposing (1) and (2) only as food for thought; dependent on
> your stance on them, I'm OK with this patch:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Thanks,
> Laszlo
>
> > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > index 8c77fc46427b..6f93032ac064 100644
> > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > @@ -28,6 +28,7 @@ [Defines]
> > # -D FLAG=VALUE
> > #
> > DEFINE TTY_TERMINAL = FALSE
> > + DEFINE NVME_ENABLE = FALSE
> > DEFINE SECURE_BOOT_ENABLE = FALSE
> > DEFINE TPM2_ENABLE = FALSE
> > DEFINE TPM2_CONFIG_ENABLE = FALSE
> > @@ -447,6 +448,13 @@ [Components.common]
> > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> >
> > + #
> > + # NVME Driver
> > + #
> > +!if $(NVME_ENABLE) == TRUE
> > + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > +!endif
> > +
> > #
> > # SMBIOS Support
> > #
> > diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > index aaba0b1c8840..45f0dd65be33 100644
> > --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > @@ -128,6 +128,13 @@ [FV.FvMain]
> > INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> >
> > + #
> > + # NVME Driver
> > + #
> > +!if $(NVME_ENABLE) == TRUE
> > + INF MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > +!endif
> > +
> > #
> > # SMBIOS Support
> > #
> > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > index 4d82a77213ec..5dd4b1cf29f4 100644
> > --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > @@ -28,6 +28,7 @@ [Defines]
> > # -D FLAG=VALUE
> > #
> > DEFINE TTY_TERMINAL = FALSE
> > + DEFINE NVME_ENABLE = FALSE
> > DEFINE SECURE_BOOT_ENABLE = FALSE
> >
> > #
> > @@ -382,6 +383,13 @@ [Components.common]
> > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> >
> > + #
> > + # NVME Driver
> > + #
> > +!if $(NVME_ENABLE) == TRUE
> > + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > +!endif
> > +
> > #
> > # SMBIOS Support
> > #
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu*
2020-04-09 14:53 ` Leif Lindholm
@ 2020-04-09 17:43 ` Leif Lindholm
0 siblings, 0 replies; 7+ messages in thread
From: Leif Lindholm @ 2020-04-09 17:43 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: devel, Ard Biesheuvel, Leendert van Doorn
On Thu, Apr 09, 2020 at 15:53:59 +0100, Leif Lindholm wrote:
> > > [Conditionalised driver inclusion]
> > > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > > ---
> > > ArmVirtPkg/ArmVirtQemu.dsc | 8 ++++++++
> > > ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 7 +++++++
> > > ArmVirtPkg/ArmVirtQemuKernel.dsc | 8 ++++++++
> > > 3 files changed, 23 insertions(+)
> >
> > (2) I realize I'm being arbitrary, with regard to what driver should be
> > conditional and what should always be there. But in OvmfPkg,
> > NvmExpressDxe is included unconditionally, and I think that should work
> > here too.
> >
> > Of course, if you *want* to make NvmExpressDxe excludable from the
> > ArmVirtQemu* platforms, then I'm fine with that.
>
> I don't. I just succumbed to premature optimization.
> Do you want to see a v2, or do I drop the bits before submission?
>
> Regards,
>
> Leif
>
> > So, I'm proposing (1) and (2) only as food for thought; dependent on
> > your stance on them, I'm OK with this patch:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Given Ard's stated opinion in other email, I'm taking this and
dropping the conditionals.
Pushed as a5d8a399635d.
Thanks!
> >
> > Thanks,
> > Laszlo
> >
> > > diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> > > index 8c77fc46427b..6f93032ac064 100644
> > > --- a/ArmVirtPkg/ArmVirtQemu.dsc
> > > +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> > > @@ -28,6 +28,7 @@ [Defines]
> > > # -D FLAG=VALUE
> > > #
> > > DEFINE TTY_TERMINAL = FALSE
> > > + DEFINE NVME_ENABLE = FALSE
> > > DEFINE SECURE_BOOT_ENABLE = FALSE
> > > DEFINE TPM2_ENABLE = FALSE
> > > DEFINE TPM2_CONFIG_ENABLE = FALSE
> > > @@ -447,6 +448,13 @@ [Components.common]
> > > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > >
> > > + #
> > > + # NVME Driver
> > > + #
> > > +!if $(NVME_ENABLE) == TRUE
> > > + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > > +!endif
> > > +
> > > #
> > > # SMBIOS Support
> > > #
> > > diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > > index aaba0b1c8840..45f0dd65be33 100644
> > > --- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > > +++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
> > > @@ -128,6 +128,13 @@ [FV.FvMain]
> > > INF MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > > INF MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > >
> > > + #
> > > + # NVME Driver
> > > + #
> > > +!if $(NVME_ENABLE) == TRUE
> > > + INF MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > > +!endif
> > > +
> > > #
> > > # SMBIOS Support
> > > #
> > > diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > index 4d82a77213ec..5dd4b1cf29f4 100644
> > > --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> > > @@ -28,6 +28,7 @@ [Defines]
> > > # -D FLAG=VALUE
> > > #
> > > DEFINE TTY_TERMINAL = FALSE
> > > + DEFINE NVME_ENABLE = FALSE
> > > DEFINE SECURE_BOOT_ENABLE = FALSE
> > >
> > > #
> > > @@ -382,6 +383,13 @@ [Components.common]
> > > MdeModulePkg/Bus/Scsi/ScsiBusDxe/ScsiBusDxe.inf
> > > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDiskDxe.inf
> > >
> > > + #
> > > + # NVME Driver
> > > + #
> > > +!if $(NVME_ENABLE) == TRUE
> > > + MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressDxe.inf
> > > +!endif
> > > +
> > > #
> > > # SMBIOS Support
> > > #
> > >
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-09 17:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-09 12:10 [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu* Leif Lindholm
2020-04-09 13:07 ` Ard Biesheuvel
2020-04-09 14:31 ` Leif Lindholm
2020-04-09 14:36 ` Ard Biesheuvel
2020-04-09 13:56 ` Laszlo Ersek
2020-04-09 14:53 ` Leif Lindholm
2020-04-09 17:43 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox