Re: [PATCH v3] hw/pci/pci_bridge: ensure PCIe slots have only one slot

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

On 7/20/22 14:04, Daniel P. Berrangé wrote:
> On Wed, Jul 20, 2022 at 02:00:16PM +0300, Roman Kagan wrote:
>> On Wed, Jul 20, 2022 at 11:44:26AM +0100, Daniel P. Berrangé wrote:
>>> On Wed, Jul 20, 2022 at 01:25:55PM +0300, Roman Kagan wrote:
>>>> It's possible to create non-working configurations by attaching a device
>>>> to a derivative of PCIe slot (pcie-root-port, ioh3420, etc) and
>>>> specifying a slot number other that zero, e.g.:
>>>>
>>>>      -device pcie-root-port,id=s0,... \
>>>>      -device virtio-blk-pci,bus=s0,addr=4,...
>>>>
>>>> Make QEMU reject such configurations and only allow addr=0 on the
>>>> secondary bus of a PCIe slot.
>>>
>>> What do you mean by 'non-working' in this case.  The guest OS boots
>>> OK, but I indeed don't see the device in the guest, but IIUC it was
>>> said that was just because Linux doesn't scan for a non-zero slot.
>>
>> Right.  I don't remember if it was Linux or firmware or both but indeed
>> at least Linux guests don't see devices if attached to a PCIe slot at
>> addr != 0.  (Which is kinda natural for a thing called "slot", isn't it?)
> 
> I vaguely recall there was an option to tell linux to scan all slots,
> not just slot 0, not sure if that's applicable here.
> 
>>
>>> That wouldn't be a broken config from QEMU's POV though, merely a
>>> guest OS limitation ?
>>
>> Strictly speaking it wouldn't, indeed.  But we've had created such a
>> configuration (due to a bug in our management layer) and spent
>> non-negligible time trying to figure out why the attached device didn't
>> appear in the guest.  So I thought it made sense to reject a
>> configuration which is known to confuse guests.  Doesn't it?
> 
> If a configuration is a permissible per the hardware design / spec, then
> QEMU should generally allow it.  We don't want to constrain host side
> configs based on the current limitations of guest OS whose behaviour can
> change over time, or where a different guest OS may have a different POV.
> 

If I understand correctly further answers the configration that we try to forbid is not permissible by PCIe spec. So seems valid to forbid it. We still need to mention specification in commit message and in the comment.

If we still afraid to forbid at once that invalid configuration that was previously allowed, may be we can proceed with some of the following:

1. Make a deprecation period of three releases and print only warning during this period. And forbid the invalid configuration three releases later. Still I'm not sure that someone will see these warnings in logs..

2. Make a boolean config option, like CONFIG_PCIE_STRICT, which forbids invalid configurations. This way we keep default behavior, that allows to test something unusual, but add an option that we can use for production solution where it's important to reduce number of possibilities to break the VM.

What do you think?

-- 
Best regards,
Vladimir