[Xen-devel] printf formatter for pci_sbdf_t

[Xen-devel] printf formatter for pci_sbdf_t

[Roger Pau Monné]

Hello,

As part of some PCI related cleanup I'm doing, which includes
expanding the usage of pci_sbdf_t, I'm also planning to add a custom
printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
without having to specify four formatters (and pass four parameters)
each time (%04x:%02x:%02x.%u).

There's been some debate on the previous version about whether the
formatter should be %pp or %op, and I would like to settle on one of
them before sending a new version:

Using %pp pros:
 - Xen already overloads p with other custom implementations.

Using %pp cons:
 - Passes a pointer (which is always 64b on x86) to store a
   32bit value (SBDF).
 - Requires a dereference to access the value.

Using %op pros:
 - Can pass a 32bit integer naturally.

Using %op cons:
 - No other overloads of the o specifier exists so far, either in Xen
   or in Linux AFAIK.

My first implementation used %pp because it's inline with the current
overloads already present, and printk not being performance critical I
don't see much problem in using 64bit to pass a 32bit value, or in
requiring a dereference to access it. We could keep using %pp and
casting the sbdf value to 'void *' to avoid the dereference, but I
don't think there's much value on doing that, the more that call sites
would need to use a macro to hide the casting away.

Anyway, I would like to get some consensus on which path to follow,
either %pp or %op before sending a new version of the series. I'm
Ccing both Andrew and Jan as they had strong opinions, and I would
personally vote for %pp as I've expressed above, but don't mind
implementing something else as long as there's consensus and it's not
going to get stuck on an endless argument.

Thanks, Roger.

[0] https://patchew.org/Xen/20190510161056.48648-1-roger.pau@citrix.com/20190510161056.48648-5-roger.pau@citrix.com/

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] printf formatter for pci_sbdf_t

[Andrew Cooper]

On 17/07/2019 15:08, Roger Pau Monné wrote:
> Hello,
>
> As part of some PCI related cleanup I'm doing, which includes
> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
> without having to specify four formatters (and pass four parameters)
> each time (%04x:%02x:%02x.%u).
>
> There's been some debate on the previous version about whether the
> formatter should be %pp or %op, and I would like to settle on one of
> them before sending a new version:

I am firmly opposed to overloading %o.

Nothing about PCI coordinates has anything to do with octal
representation; its mostly hex.  And for the record, I'm firmly opposed
to overloading %[xuid] as well.

%p is the only formatter which has magic overloading so far, and
avoiding gaining a second would be of substantial value when it comes to
reading the code.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] printf formatter for pci_sbdf_t

[Jan Beulich]

On 17.07.2019 19:06, Andrew Cooper wrote:
> On 17/07/2019 15:08, Roger Pau Monné wrote:
>> As part of some PCI related cleanup I'm doing, which includes
>> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
>> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
>> without having to specify four formatters (and pass four parameters)
>> each time (%04x:%02x:%02x.%u).
>>
>> There's been some debate on the previous version about whether the
>> formatter should be %pp or %op, and I would like to settle on one of
>> them before sending a new version:
> 
> I am firmly opposed to overloading %o.

And I am firmly of the opinion that using %o for SBDF is the more
natural thing to do.

> Nothing about PCI coordinates has anything to do with octal
> representation; its mostly hex.

The domain and vCPU IDs aren#t pointer-ish either, for example.

> And for the record, I'm firmly opposed to overloading %[xuid] as well.

I agree that we don#t want to overload any of these.

> %p is the only formatter which has magic overloading so far, and
> avoiding gaining a second would be of substantial value when it comes to
> reading the code.

I don't buy this argument. Readability of some of the printk()
invocations in Roger's patch was (severely imo) hampered by the need
to take addresses of things that could be easily passed by value.
Generated code (size) should be taken into consideration here too,
as should be the (slightly) larger stack consumption when going the
%pp route.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] printf formatter for pci_sbdf_t

[Roger Pau Monné]

On Thu, Jul 18, 2019 at 11:51:57AM +0000, Jan Beulich wrote:
> On 17.07.2019 19:06, Andrew Cooper wrote:
> > On 17/07/2019 15:08, Roger Pau Monné wrote:
> >> As part of some PCI related cleanup I'm doing, which includes
> >> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
> >> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
> >> without having to specify four formatters (and pass four parameters)
> >> each time (%04x:%02x:%02x.%u).
> >>
> >> There's been some debate on the previous version about whether the
> >> formatter should be %pp or %op, and I would like to settle on one of
> >> them before sending a new version:
> > 
> > I am firmly opposed to overloading %o.
> 
> And I am firmly of the opinion that using %o for SBDF is the more
> natural thing to do.
> 
> > Nothing about PCI coordinates has anything to do with octal
> > representation; its mostly hex.
> 
> The domain and vCPU IDs aren#t pointer-ish either, for example.

But we do pass the vcpu and the domain struct pointer to those
modifiers? ie: we don't pass a vcpu or a domain ID directly in either
case.

> > And for the record, I'm firmly opposed to overloading %[xuid] as well.
> 
> I agree that we don#t want to overload any of these.
> 
> > %p is the only formatter which has magic overloading so far, and
> > avoiding gaining a second would be of substantial value when it comes to
> > reading the code.
> 
> I don't buy this argument. Readability of some of the printk()
> invocations in Roger's patch was (severely imo) hampered by the need
> to take addresses of things that could be easily passed by value.
> Generated code (size) should be taken into consideration here too,
> as should be the (slightly) larger stack consumption when going the
> %pp route.

I personally don't think passing the address instead of the value (ie:
adding an extra &) matters that much. If we pass by value then we
would have to explicitly pass the sbdf field of the struct I guess?
Which again seems to differ from what we do for vcpus and domains,
where a pointer to the struct is passed, regardless of whether the
actually formatter only cares about the ID.

AFAICT both you an Andrew agree that a custom printf formatter for PCI
SBDF is a desirable thing to have, the only disagreement is on the
actual implementation detail (whether %pp or %op should be used). In
which case I think I would like to call for THE REST to also voice in
their opinion on the matter in order to try to resolve the situation
so that I can make progress on the series, sorry if this is awkward.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] printf formatter for pci_sbdf_t

[Jan Beulich]

On 18.07.2019 17:14, Roger Pau Monné  wrote:
> On Thu, Jul 18, 2019 at 11:51:57AM +0000, Jan Beulich wrote:
>> On 17.07.2019 19:06, Andrew Cooper wrote:
>>> On 17/07/2019 15:08, Roger Pau Monné wrote:
>>>> As part of some PCI related cleanup I'm doing, which includes
>>>> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
>>>> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
>>>> without having to specify four formatters (and pass four parameters)
>>>> each time (%04x:%02x:%02x.%u).
>>>>
>>>> There's been some debate on the previous version about whether the
>>>> formatter should be %pp or %op, and I would like to settle on one of
>>>> them before sending a new version:
>>>
>>> I am firmly opposed to overloading %o.
>>
>> And I am firmly of the opinion that using %o for SBDF is the more
>> natural thing to do.
>>
>>> Nothing about PCI coordinates has anything to do with octal
>>> representation; its mostly hex.
>>
>> The domain and vCPU IDs aren#t pointer-ish either, for example.
> 
> But we do pass the vcpu and the domain struct pointer to those
> modifiers? ie: we don't pass a vcpu or a domain ID directly in either
> case.

Right. My point was: The actual format specifier used ('p' or 'o')
doesn't necessarily have anything to do with the resulting output.

> AFAICT both you an Andrew agree that a custom printf formatter for PCI
> SBDF is a desirable thing to have, the only disagreement is on the
> actual implementation detail (whether %pp or %op should be used). In
> which case I think I would like to call for THE REST to also voice in
> their opinion on the matter in order to try to resolve the situation
> so that I can make progress on the series, sorry if this is awkward.

That's perfectly fine with me. If a majority agrees with Andrew,
so be it.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] printf formatter for pci_sbdf_t

[Jan Beulich]

On 17.07.2019 19:06, Andrew Cooper wrote:
> On 17/07/2019 15:08, Roger Pau Monné wrote:
>> Hello,
>>
>> As part of some PCI related cleanup I'm doing, which includes
>> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
>> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
>> without having to specify four formatters (and pass four parameters)
>> each time (%04x:%02x:%02x.%u).
>>
>> There's been some debate on the previous version about whether the
>> formatter should be %pp or %op, and I would like to settle on one of
>> them before sending a new version:
> 
> I am firmly opposed to overloading %o.

There's actually one more argument against %p: Previously you
did express a desire for our %p extensions to stay somewhat in
sync with Linux'es. Linux is using quite a few of the available
characters already, so there's an increasing risk of conflicts.
Using %o here would risk a future conflict only if Linux
eventually decided to also overload %o.

And one more remark: If we were to introduce a more generic
"device identifier" extension, then I'd obviously be fine with
using %p. After all we'd then want to pass in struct device *.
I'm actually surprised Linux doesn't already have such a generic
formatter.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] printf formatter for pci_sbdf_t

[George Dunlap]

On Wed, Jul 17, 2019 at 3:08 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> Hello,
>
> As part of some PCI related cleanup I'm doing, which includes
> expanding the usage of pci_sbdf_t, I'm also planning to add a custom
> printf formatter for pci_sbdf_t [0], so that a SBDF can be printed
> without having to specify four formatters (and pass four parameters)
> each time (%04x:%02x:%02x.%u).
>
> There's been some debate on the previous version about whether the
> formatter should be %pp or %op, and I would like to settle on one of
> them before sending a new version:
>
> Using %pp pros:
>  - Xen already overloads p with other custom implementations.
>
> Using %pp cons:
>  - Passes a pointer (which is always 64b on x86) to store a
>    32bit value (SBDF).
>  - Requires a dereference to access the value.
>
> Using %op pros:
>  - Can pass a 32bit integer naturally.
>
> Using %op cons:
>  - No other overloads of the o specifier exists so far, either in Xen
>    or in Linux AFAIK.
>
> My first implementation used %pp because it's inline with the current
> overloads already present, and printk not being performance critical I
> don't see much problem in using 64bit to pass a 32bit value, or in
> requiring a dereference to access it. We could keep using %pp and
> casting the sbdf value to 'void *' to avoid the dereference, but I
> don't think there's much value on doing that, the more that call sites
> would need to use a macro to hide the casting away.

This is missing a bit more background needed to understand what's going on.

In particular, we use gcc's -Wformat option to "check" the values for
printk and other printf-like functions.  gcc will only allow values
that it knows about.

This check is meant to increase security, but in fact, all of the
"extended" %p format are driving a big truck through this; to gcc,
"%pd" looks like it should print "<pointer value>d".  gcc can check
that *a pointer* was passed, but not *the right pointer*; so it's
entirely possible to make a mistake where you pass a complex pointer
and cause a buffer overflow or other issues.  And as the "extended"
pointer thing may print an arbitrary number of bytes, it completely
throws off any sprintf-style buffer size checking.

gcc already has lots of -Wformat* features; the very first time Linux
wanted to do this 15 years ago or whenever, they should have gone to
the gcc guys and asked for this type of formatting to be explicitly
supported and type checked.  But anyway, here we are.

Given how much what we do undermines the safety which -Wformat
provides, I think it is worth deciding whether it's something we
actually need.

Andy objects to the idea of using "%o" for anything other than
displaying octal.  I agree with that.  Furthermore, I think "%p"
should *only* be used for things which are actually pointers, for
basically the same reason.  If we use "%pp" or something like it, it
should pass a pointer, not a value cast to a (void *).

The idea behind passing-by-value rather than passing-by-reference
seems to be to be able to construct a value without having to create
one on the stack.  I agree this is a more satisfying thing to do, but
printk is already normally sending out bits over a serial line; I
don't think an extra word on the stack and an extra dereference of a
hot pointer is really that big of a win.

I would personally be OK with extending '%x', since 'x' to me
indicates direct visualization of a binary value.  But neither Jan nor
Andy seem to like that option, and I don't feel like pushing it.

Which leaves two options, AFAICT:

1. Use %pp (or something like it), and pass a pointer
2. Disable -Wformat, at least until such time as it supports extending
the format checking.

Andy seems keen on having "%p" be the only overloaded operator.  I
disagree; I don't mind overloading other operators, but I agree that
"%o" isn't a good fit. I think "%p" should be only for pointers.

Staying in sync with Linux isn't a big issue; if they end up using
"%pp", we can just switch to a different letter / identifier.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel