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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Daniel P. Berrangé <berrange@xxxxxxxxxx> writes:

> On Tue, Oct 27, 2026 at 03:54:03PM +0100, Markus Armbruster wrote:
>> Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> writes:
>> 
>> > Hi
>> >
>> > On Fri, Oct 23, 2026 at 2:04 PM Daniel P. Berrangé <berrange@xxxxxxxxxx> 
>> > 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.

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.

> 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.