Re: [PATCH v5 05/11] drm: nova: Add an info ioctl

[Alistair Popple <apopple@nvidia.com>]

On 2026-09-02 at 15:21 +1000, Alistair Popple <apopple@nvidia.com> wrote...
> On 2026-09-01 at 17:29 +1000, Danilo Krummrich <dakr@kernel.org> wrote...
> > On Tue Sep 1, 2026 at 7:09 AM CEST, Alistair Popple wrote:
> > > On 2026-09-01 at 14:50 +1000, Dave Airlie <airlied@gmail.com> wrote...
> > >> > >
> > >> > > We should make the NovaCoreApi just provide an accessor for &Spec and make every
> > >> > > subsequent method we need public. Otherwise we end up with endless forwarding
> > >> > > methods. We can also add as_raw() methods to the specific types as needed.
> > >> >
> > >> > Ok. This is where I don't have a good instinct for what we think should be an
> > >> > accessor/forwarding method vs. where we should just expose the underlying data
> > >> > structure and required methods to API users.
> > >> >
> > >> > In the past it seems there's been some resistance to exposing nova-core or gsp
> > >> > data structures like this which is why I added the forwarding methods. In future
> > >> > we're going to have other data-structures that NovaCoreApi will need to access
> > >> > so it would be good to understand what we should do here so we can keep things
> > >> > somewhat consistent.
> > >> 
> > >> We can expose structure defined in nova-core, we cannot expose
> > >> structures defined in gsp bindings or firmware.
> > 
> > Yes, those structures are not even exposed to the nova-core layers we are
> > dealing with here.
> > 
> > There are the structures abstracting firmware interfaces, but despite exposing
> > trivial cached values, they should not exposed to nova-drm either.
> > 
> > Instead nova-drm should call into nova-core and ask it to do things on its
> > behalf.
> > 
> > More in general, the same is true for any other device resources. For instance,
> > we also should not expose the pci::Device or the pci::Bar to nova-drm, but again
> > provide a higher level API.
> > 
> > > All the fields of self.gpu are structures defined as rust native structures in
> > > nova-core. Their values may be decoded or derived from GSP responses, but by
> > > design none of the raw structures from gsp bindings live in self.gpu AFAIK.
> > 
> > We could indeed expose all values from struct Gpu directly and control
> > everything through visibility, but ...
> > 
> > >> In theory we can internally between core/drm but I'd really really
> > >> like to keep that boundary as the limits of GSP for auditability
> > >> purposes.
> > 
> > ... exposing struct Gpu and controlling everything through visibility still
> > means that we could easily miss some device resource being exposed directly to
> > nova-drm, which also includes GSP abstractions.
> 
> The individual fields would still need to be explicitly marked as `pub`, so it's
> not like just exposing the top-level data structure suddenly exposes all these
> device resources without them being explicitly marked as accessible.
> 
> > So, I think we should provide accessors for the major structures that we
> > intentionally want to expose (so we have an obvious boundary), but not for every
> > single fields inside them.
> 
> Sure, I don't really mind either way. I just figured that's what the point
> of Rust visibiity was, to provide a simple boiler-plate free way of saying a
> type/method/field was used externally. But will switch this to using accessors
> for major data structures instead given that seems preferred.

That said with the changes required for Invariant Gpu and being able to project
Covariant fields out I can see why accessors for top-level data structures might
be useful, particularly if some of them (eg. Spec) are Covariant.

> > This is also why I said that we can expose an accessor for struct Spec, which I
> > think serves as a good example.
> 
> Ok, so just to double check are you happy with an accessor to return
> gsp_static_info as well?

Still interested in your thoughts here though, just to avoid churn because I'll
probably pick the wrong answer :-)

 - Alistair

> Thanks.
> 
>  - Alistair