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

[Alistair Popple]

On 2026-09-01 at 17:29 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> 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@xxxxxxxxx> 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.
 
> 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?

Thanks.

 - Alistair