Re: [PATCH v7 54/65] qdev: adjust PciDevfn declared type

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Oct 28, 2026 at 08:13:44AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Oct 27, 2026 at 03:54:03PM +0100, Markus Armbruster wrote:
> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >> 
> >> > Hi
> >> >
> >> > On Fri, Oct 23, 2026 at 2:04 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >> >>
> >> >> On Tue, Oct 20, 2026 at 02:46:11PM +0400, Marc-André Lureau wrote:
> >> >> > PciDevfn is a bit special, since it accepts integer and string as
> >> >> > setter, but only output integer as getter.
> >
> > snip
> >
> >> >                                                       But I agree we
> >> > could do better.
> >> >
> >> > There are multiple options. We could keep declaring the property as
> >> > str instead. In this case, the getter should probably return a string
> >> > for consistency and correctness. This might surprise some existing
> >> > users though.
> >> 
> >> I.e. ditch numeric suppport for numeric addresses, right?
> >> 
> >> Incompatible change.  Do we care?
> >> 
> >> If we don't, we should be honest about it and mark qom-get unstable.
> >> 
> >> > We could introduce new properties like `devfn-str` and `devfn-int`
> >> > that alias the same value.
> >> 
> >> Compatibility break.  If breaking compatibility is okay, I'd rather use
> >> the opportunity to get rid of cleverness, and go with your first option.
> >> 
> >> > We could also introduce a new structured QAPI type.
> >> 
> >> What kind of type do you have in mind?
> >
> > IMHO the root cause design flaw here was to overload 'addr' as a
> > way to set both the slot number and function number, as using
> > one property for two pieces of data is a well known anti-pattern
> > we caution against.
> >
> > IOW, we should have had 'slot' and 'function' as separate properties
> > from the start.
> 
> Fair.
> 
> Our modeling of PCI is old and weird.
> 
> What we call "pci-device" isn't a device, it's a function, where
> function 0 sort of stands in for the device.

Mmm, yes, interesting way of looking at it.

> Its "addr" isn't a PCI address, it's the address of the function on its
> PCI bus, i.e. the tail of a PCI address.
> 
> > From a CLI POV, we could easily change 'addr' to mean only the slot
> > number, since it is always parsed in string format without the
> > magic (slot << 4) | function encoding. Then add a 'function' prop
> 
> In CLI, HMP and QMP string (not integer), value "5" is shorthand for
> "5.0", and means device 5, function 0.
> 
> We could deprecate the .FN part in favor of a new property "function".
> 
> Once we drop support for .FN from "addr", its name becomes even more
> misleading than it always was.

Yes.

> > The challenge is the impact on QMP with strict typing, as ideally
> > int format would soley represent the slot number, but today the
> > int format is encoded (slot << 4) | function.  So anyone relying
> > on int format in QMP would silently have a semantic change if we
> > altered what 'addr' means. 
> 
> That's a no-go.
> 
> > Deprecating 'addr' entirely and adding brand new 'slot' and
> > 'function' properties would be a significant disruption to
> > a syntax which has been documented for years (decades).
> 
> What about this:
> 
> * No change to PCIDevice member @devfn: encodes slot and function as an
>   integer.
> 
> * New "pci-device" properties "slot" and "function".  "slot" sets just
>   the slot bits of @devfn, and "function" sets just the function bits.
>   Both default to zero, so specifying just "slot" gives you
>   PCI_DEVFN(slot, 0), which makes sense.
> 
> * Deprecate "pci-device" property "addr" for QMP.  We can keep it around
>   for longer than the required grace period if that helps.
> 
> * Declare it legacy for CLI and HMP, with no intent to remove it.

We can't easily keep it in CLI/HMP, while removing from QMP can we ?
As long as the property exists on the PCIDevice object, it will be
accessible everywhere.


Perhaps keep it relatively simple and merely deprecate the string
syntax of 'addr' in QMP. That's the place where it is really ugy.
Force QMP over to only the integer syntax for 'addr', and leave
CLI/HMP accepting the (optionally dotted) string syntax.

Mgmt apps like libvirt will have to adapt. Tedious, but actually
quite simple compared to most deprecations, as the integer syntax
can already be used with all historical QEMU versions in QMP that
mgmt apps ever need to deal with. They can unconditionally change
their code to int syntax and not worry about old/new QEMUs at all.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|