From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.10629.1586444043951547783 for ; Thu, 09 Apr 2020 07:54:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=jPJiVGeF; spf=pass (domain: nuviainc.com, ip: 209.85.221.68, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f68.google.com with SMTP id k1so12310284wrm.3 for ; Thu, 09 Apr 2020 07:54:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=6MkPb5MGjpLs6c/Twd7EbrLYpoGsr1XPZcm82Iy7O44=; b=jPJiVGeFj64drfkAYgDdjVT062FbdsW4KxVEQOAJt8uBzmxk7DHoHu36FxV5oFe1Qi 3rU+qjq+DoGWAHh339J2UvDBeXTQ9Gk5upa6uqIcTmgNJHs+RljPZbF1aeZr9g/WAPS6 MP5fNSSE+yzeyn0BKCNtrDdWv0Ytc5LyybbHbiIdS9m+kv4mWHz/jBNoOgSOmfoAnF64 7Cgd89Wn8zpHzcKIEqAzKg+Xu3hwF7nF+U/tIvM4+FkftuwvcRtBvGR0KNwM4qFCBAfj zIpkPxtFXS+WHuHuOYDyAMmxbEEDa0OE5rw6hgGW6oEJ0QdbKL+YUnYX4SiQ3+3aoaHy DtJA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=6MkPb5MGjpLs6c/Twd7EbrLYpoGsr1XPZcm82Iy7O44=; b=kvfkHF+gqzuUwxlJ/b2qTFEOxwdh7SmEQ4bdd4hwSbixWeRYCnxR82CTBLUv59oYiT G2ubtJguCzrCVd6Y8Rn2bVsTatUgrVFLX4JjtpZCwXVIkyFJDz3+47WeHzEBbjNjpUX4 5zBsu3m6ZQfx67GZd6vginjPPBwi8G5kW1HH5qEd8nZgYSBeFClKVAbKP7/eHwwoWdPn Zf10lJO0+nRZ4QqGPybzxn2j0zzEeOSRD22pF3h/Yqw1WXAqHenkYak40hUYRb3mz7Pw gVhoJPvKyj2QYsgOxt0EvNuVkBGy0PSnkZix8KWw8aGGyN8GQn0h4o+LvJC9hKIQky6u UXuw== X-Gm-Message-State: AGi0PuaHNTF+9wwspxXi+uboOKjCvyX07i2Bl6368FjoznrsGupPhvqI HvIETbcG9lLtVdjX6vkwjI0zHw== X-Google-Smtp-Source: APiQypLs9+zVcOdL3zADUHZ8KBNnDICbmJjR0KuW0FcV9sxSGwiAuFS1q7c/8vi/04y/Wq8aqNc3Nw== X-Received: by 2002:a5d:498b:: with SMTP id r11mr14584556wrq.368.1586444042555; Thu, 09 Apr 2020 07:54:02 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id h2sm24719925wrp.50.2020.04.09.07.54.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Apr 2020 07:54:02 -0700 (PDT) Date: Thu, 9 Apr 2020 15:53:59 +0100 From: "Leif Lindholm" To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Leendert van Doorn Subject: Re: [PATCH 1/1] ArmVirtPkg: Include NVMe support in ArmVirtQemu* Message-ID: <20200409145359.GD14075@vanye> References: <20200409121050.15387-1-leif@nuviainc.com> <728eab99-1b9f-6fc8-c566-b5ef6f8b5c0b@redhat.com> MIME-Version: 1.0 In-Reply-To: <728eab99-1b9f-6fc8-c566-b5ef6f8b5c0b@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > > > > 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 > > --- > > 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 > > 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 > > # > > >