Re: [PATCH 2/2] sd: disable sdhci-pci device by default - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>, P J P <ppandit@redhat.com>
Cc: Prasad J Pandit <pjp@fedoraproject.org>,
	Emanuele Giuseppe Esposito <e.emanuelegiuseppe@gmail.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Alexander Bulekov <alxndr@bu.edu>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH 2/2] sd: disable sdhci-pci device by default
Date: Wed, 20 May 2020 18:33:19 +0200	[thread overview]
Message-ID: <431930bb-ac30-62fb-11f0-c388c0e9c763@redhat.com> (raw)
In-Reply-To: <CAFEAcA_WbJR9PWpw4f2jWecouSn7U0y9=0t4ek1rGwxtM6tXBQ@mail.gmail.com>

+Kevin, Paolo, Emanuele

On 5/20/20 5:39 PM, Peter Maydell wrote:
> On Wed, 20 May 2020 at 16:28, P J P <ppandit@redhat.com> wrote:
>>
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> Disable rarely used sdhci-pci device build by default.
>>
>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
> 
> Doesn't this break existing working command lines? The
> device exists, some people use it. We should treat it like
> other PCI devices -- if the guest arch/machine can handle
> PCI the device should be built.

Prasad, I once tried to remove it, and Kevin said he was using it:

https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02765.html

   I do find qemu's PCI SDHCI support useful for testing.
   SeaBIOS can launch an OS from PCI SDHCI (qemu-system-x86_64
   -device sdhci-pci -device sd-card,drive=drive0 -drive
   id=drive0,if=none,file=dos-drivec) and linux has drivers for
   it as well.  A number of the Chromebooks ship with PCI SDHCI
   devices on them, so it's not an unheard of configuration.

> 
> There's obviously scope for being more general and allowing
> some kind of "only build the subset of devices we feel
> more confident abut the security of" setup (don't RH do
> something like this downstream?), but upstream we don't
> have a concept like that, we just build everything.

Prasad, again back at that time I tried to remove this (as the device 
appears unused) Paolo told me after asking explanation for his comment 
"PCI devices can be created with -device, they don't have to be added by
boards." [*] - I guess it was on IRC - to check commit 224d10ff5ae, this 
device was added with RH PCI ID because it was useful for testing:

static void sdhci_pci_class_init(ObjectClass *klass, void *data)
{
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);

     k->init = sdhci_pci_init;
     k->exit = sdhci_pci_exit;
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     ...

This device is also nicely used as example for the qgraph testing (see 
tests/test-qgraph.c added in fc281c80202).

[*] https://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg02819.html

Peter, indeed the Kconfig was added to allow distributions to disable 
piece of code, and we want to keep this device in mainstream QEMU.
Distributions are free to disable it setting SDHCI_PCI=n

So to this patch:

Nack.

> 
> thanks
> -- PMM
> 