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

[Igor Mammedov]

On Mon, 25 Jul 2022 16:59:21 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:

> 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?
Given that non zero slots are used only on broken hardware/firmware and it's
just workaround for those in linux kernel.
I wouldn't bother with 1 or 2 (I think just a note on change log should
be sufficient)