[PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Alistair Popple]

Update the safety comments to be consistent with other safety comments
in the PCI bindings. Also add an inline compiler hint.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Alex Gaynor <alex.gaynor@gmail.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Gary Guo <gary@garyguo.net>
Cc: "Björn Roy Baron" <bjorn3_gh@protonmail.com>
Cc: Benno Lossin <lossin@kernel.org>
Cc: Andreas Hindborg <a.hindborg@kernel.org>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Trevor Gross <tmgross@umich.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---

Changes for v2:

 - New for v2
---
 rust/kernel/pci.rs | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..5c35a66a5251 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
 
 impl Device {
     /// Returns the PCI vendor ID.
+    #[inline]
     pub fn vendor_id(&self) -> u16 {
-        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
+        // `struct pci_dev`.
         unsafe { (*self.as_raw()).vendor }
     }
 
     /// Returns the PCI device ID.
+    #[inline]
     pub fn device_id(&self) -> u16 {
-        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
+        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
+        // `struct pci_dev`.
         unsafe { (*self.as_raw()).device }
     }
 
-- 
2.47.2

[PATCH v2 2/2] rust: Add several miscellaneous PCI helpers

[Alistair Popple]

Add bindings to obtain a PCI device's resource start address, bus/
device function, revision ID and subsystem device and vendor IDs.

These will be used by the nova-core GPU driver which is currently in
development.

Signed-off-by: Alistair Popple <apopple@nvidia.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Alex Gaynor <alex.gaynor@gmail.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Gary Guo <gary@garyguo.net>
Cc: "Björn Roy Baron" <bjorn3_gh@protonmail.com>
Cc: Benno Lossin <lossin@kernel.org>
Cc: Andreas Hindborg <a.hindborg@kernel.org>
Cc: Alice Ryhl <aliceryhl@google.com>
Cc: Trevor Gross <tmgross@umich.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Alexandre Courbot <acourbot@nvidia.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---

Changes for v2:
 - Update comments and add inline compiler hint.
 - Add note for where these will be used.
---
 rust/helpers/pci.c | 10 ++++++++++
 rust/kernel/pci.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/rust/helpers/pci.c b/rust/helpers/pci.c
index cd0e6bf2cc4d..59d15bd4bdb1 100644
--- a/rust/helpers/pci.c
+++ b/rust/helpers/pci.c
@@ -12,6 +12,16 @@ void *rust_helper_pci_get_drvdata(struct pci_dev *pdev)
 	return pci_get_drvdata(pdev);
 }
 
+u16 rust_helper_pci_dev_id(struct pci_dev *dev)
+{
+	return PCI_DEVID(dev->bus->number, dev->devfn);
+}
+
+resource_size_t rust_helper_pci_resource_start(struct pci_dev *pdev, int bar)
+{
+	return pci_resource_start(pdev, bar);
+}
+
 resource_size_t rust_helper_pci_resource_len(struct pci_dev *pdev, int bar)
 {
 	return pci_resource_len(pdev, bar);
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 5c35a66a5251..25f5693f32d6 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -386,6 +386,50 @@ pub fn device_id(&self) -> u16 {
         unsafe { (*self.as_raw()).device }
     }
 
+    /// Returns the PCI revision ID.
+    #[inline]
+    pub fn revision_id(&self) -> u8 {
+        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
+        // `struct pci_dev`.
+        unsafe { (*self.as_raw()).revision }
+    }
+
+    /// Returns the PCI bus device/function.
+    #[inline]
+    pub fn dev_id(&self) -> u16 {
+        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
+        // `struct pci_dev`.
+        unsafe { bindings::pci_dev_id(self.as_raw()) }
+    }
+
+    /// Returns the PCI subsystem vendor ID.
+    #[inline]
+    pub fn subsystem_vendor_id(&self) -> u16 {
+        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
+        // `struct pci_dev`.
+        unsafe { (*self.as_raw()).subsystem_vendor }
+    }
+
+    /// Returns the PCI subsystem device ID.
+    #[inline]
+    pub fn subsystem_device_id(&self) -> u16 {
+        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
+        // `struct pci_dev`.
+        unsafe { (*self.as_raw()).subsystem_device }
+    }
+
+    /// Returns the start of the given PCI bar resource.
+    pub fn resource_start(&self, bar: u32) -> Result<bindings::resource_size_t> {
+        if !Bar::index_is_valid(bar) {
+            return Err(EINVAL);
+        }
+
+        // SAFETY:
+        // - `bar` is a valid bar number, as guaranteed by the above call to `Bar::index_is_valid`,
+        // - by its type invariant `self.as_raw` is always a valid pointer to a `struct pci_dev`.
+        Ok(unsafe { bindings::pci_resource_start(self.as_raw(), bar.try_into()?) })
+    }
+
     /// Returns the size of the given PCI bar resource.
     pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
         if !Bar::index_is_valid(bar) {
-- 
2.47.2

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 8435f8132e38..5c35a66a5251 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>  
>  impl Device {
>      /// Returns the PCI vendor ID.
> +    #[inline]
>      pub fn vendor_id(&self) -> u16 {
> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a

s/by its type invariant/by the type invariants of `Self`,/
s/always//

Also, which invariant does this refer to? The only one that I can see
is:

    /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.

And this doesn't say anything about the validity of `self.as_raw()`...

> +        // `struct pci_dev`.
>          unsafe { (*self.as_raw()).vendor }
>      }
>  
>      /// Returns the PCI device ID.
> +    #[inline]
>      pub fn device_id(&self) -> u16 {
> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
> +        // `struct pci_dev`.

Ditto here.

---
Cheers,
Benno

>          unsafe { (*self.as_raw()).device }
>      }
>  

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Alistair Popple]

On Thu, Jul 10, 2025 at 10:01:05AM +0200, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 8435f8132e38..5c35a66a5251 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> >  
> >  impl Device {
> >      /// Returns the PCI vendor ID.
> > +    #[inline]
> >      pub fn vendor_id(&self) -> u16 {
> > -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> > +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
> 
> s/by its type invariant/by the type invariants of `Self`,/
> s/always//
> 
> Also, which invariant does this refer to? The only one that I can see
> is:
> 
>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.

Actually isn't that wrong? Shouldn't that read for "a valid `struct pci_dev`"?

> And this doesn't say anything about the validity of `self.as_raw()`...

Isn't it up to whatever created this pci::Device to ensure the underlying struct
pci_dev remains valid for at least the lifetime of `Self`? Sorry I'm quite new
to Rust (and especially Rust in the kernel), so not sure what the best way to
express that in a SAFETY style comment would be. Are you saying the list of
invariants for pci::Device also needs expanding?

Thanks.

> > +        // `struct pci_dev`.
> >          unsafe { (*self.as_raw()).vendor }
> >      }
> >  
> >      /// Returns the PCI device ID.
> > +    #[inline]
> >      pub fn device_id(&self) -> u16 {
> > -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> > +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
> > +        // `struct pci_dev`.
> 
> Ditto here.
> 
> ---
> Cheers,
> Benno
> 
> >          unsafe { (*self.as_raw()).device }
> >      }
> >  
> 

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Fri Jul 11, 2025 at 1:22 AM CEST, Alistair Popple wrote:
> On Thu, Jul 10, 2025 at 10:01:05AM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> > index 8435f8132e38..5c35a66a5251 100644
>> > --- a/rust/kernel/pci.rs
>> > +++ b/rust/kernel/pci.rs
>> > @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>> >  
>> >  impl Device {
>> >      /// Returns the PCI vendor ID.
>> > +    #[inline]
>> >      pub fn vendor_id(&self) -> u16 {
>> > -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> > +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>> 
>> s/by its type invariant/by the type invariants of `Self`,/
>> s/always//
>> 
>> Also, which invariant does this refer to? The only one that I can see
>> is:
>> 
>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>
> Actually isn't that wrong? Shouldn't that read for "a valid `struct pci_dev`"?

Yeah it should probably be changed, I'm not sure what exactly is
required here, but this already would be an improvement:

    /// `self.0` is a valid `struct pci_dev`.

>> And this doesn't say anything about the validity of `self.as_raw()`...
>
> Isn't it up to whatever created this pci::Device to ensure the underlying struct
> pci_dev remains valid for at least the lifetime of `Self`?

Well yes and no. It is up to the creator of this specific `pci::Device`
to ensure that it is valid, but that is true for all creators of
`pci::Device`. In other words this property doesn't change while the
`pci::Device` is alive so we call it an "invariant".

When creating a `pci::Device`, you have to ensure all invariants are met
and then anyone using it can rely on them being true.

Now in this particular instance the `as_raw` function is just calling
`self.0.get()`. I'm not sure that's worth it, since it isn't even
shorter and it makes the safety docs a bit worse. So my suggestion would
be to remove it.

> Sorry I'm quite new to Rust (and especially Rust in the kernel), so
> not sure what the best way to express that in a SAFETY style comment
> would be. Are you saying the list of invariants for pci::Device also
> needs expanding?

No worries, safety documentation is pretty hard :)

---
Cheers,
Benno

>
> Thanks.
>
>> > +        // `struct pci_dev`.
>> >          unsafe { (*self.as_raw()).vendor }
>> >      }
>> >  
>> >      /// Returns the PCI device ID.
>> > +    #[inline]
>> >      pub fn device_id(&self) -> u16 {
>> > -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> > +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>> > +        // `struct pci_dev`.
>> 
>> Ditto here.
>> 
>> ---
>> Cheers,
>> Benno
>> 
>> >          unsafe { (*self.as_raw()).device }
>> >      }
>> >  
>> 

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> index 8435f8132e38..5c35a66a5251 100644
>> --- a/rust/kernel/pci.rs
>> +++ b/rust/kernel/pci.rs
>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>>  
>>  impl Device {
>>      /// Returns the PCI vendor ID.
>> +    #[inline]
>>      pub fn vendor_id(&self) -> u16 {
>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>
> s/by its type invariant/by the type invariants of `Self`,/
> s/always//
>
> Also, which invariant does this refer to? The only one that I can see
> is:
>
>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>
> And this doesn't say anything about the validity of `self.as_raw()`...

Hm...why not? If an instance of Self always represents a valid struct pci_dev,
then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
no?

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On Fri Jul 11, 2025 at 1:22 AM CEST, Alistair Popple wrote:
> On Thu, Jul 10, 2025 at 10:01:05AM +0200, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> > index 8435f8132e38..5c35a66a5251 100644
>> > --- a/rust/kernel/pci.rs
>> > +++ b/rust/kernel/pci.rs
>> > @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>> >  
>> >  impl Device {
>> >      /// Returns the PCI vendor ID.
>> > +    #[inline]
>> >      pub fn vendor_id(&self) -> u16 {
>> > -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> > +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>> 
>> s/by its type invariant/by the type invariants of `Self`,/
>> s/always//
>> 
>> Also, which invariant does this refer to? The only one that I can see
>> is:
>> 
>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>
> Actually isn't that wrong? Shouldn't that read for "a valid `struct pci_dev`"?

Yes, and it's fixed in the driver-core tree already. :)

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>>> index 8435f8132e38..5c35a66a5251 100644
>>> --- a/rust/kernel/pci.rs
>>> +++ b/rust/kernel/pci.rs
>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>>>  
>>>  impl Device {
>>>      /// Returns the PCI vendor ID.
>>> +    #[inline]
>>>      pub fn vendor_id(&self) -> u16 {
>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>>
>> s/by its type invariant/by the type invariants of `Self`,/
>> s/always//
>>
>> Also, which invariant does this refer to? The only one that I can see
>> is:
>>
>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>>
>> And this doesn't say anything about the validity of `self.as_raw()`...
>
> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
> no?

While it's true, you need to look into the implementation of `as_raw`.
It could very well return a null pointer...

This is where we can use a `Guarantee` on that function. But since it's
not shorter than `.0.get()`, I would just remove it.

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>>>> index 8435f8132e38..5c35a66a5251 100644
>>>> --- a/rust/kernel/pci.rs
>>>> +++ b/rust/kernel/pci.rs
>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>>>>  
>>>>  impl Device {
>>>>      /// Returns the PCI vendor ID.
>>>> +    #[inline]
>>>>      pub fn vendor_id(&self) -> u16 {
>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>>>
>>> s/by its type invariant/by the type invariants of `Self`,/
>>> s/always//
>>>
>>> Also, which invariant does this refer to? The only one that I can see
>>> is:
>>>
>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>>>
>>> And this doesn't say anything about the validity of `self.as_raw()`...
>>
>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
>> no?
>
> While it's true, you need to look into the implementation of `as_raw`.
> It could very well return a null pointer...
>
> This is where we can use a `Guarantee` on that function. But since it's
> not shorter than `.0.get()`, I would just remove it.

We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
a `Guarantee` to be clean, we should probably fix it up in a treewide change.

as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
much less obvious.

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Fri Jul 11, 2025 at 9:33 PM CEST, Danilo Krummrich wrote:
> On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
>>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>>>>> index 8435f8132e38..5c35a66a5251 100644
>>>>> --- a/rust/kernel/pci.rs
>>>>> +++ b/rust/kernel/pci.rs
>>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>>>>>  
>>>>>  impl Device {
>>>>>      /// Returns the PCI vendor ID.
>>>>> +    #[inline]
>>>>>      pub fn vendor_id(&self) -> u16 {
>>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>>>>
>>>> s/by its type invariant/by the type invariants of `Self`,/
>>>> s/always//
>>>>
>>>> Also, which invariant does this refer to? The only one that I can see
>>>> is:
>>>>
>>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>>>>
>>>> And this doesn't say anything about the validity of `self.as_raw()`...
>>>
>>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
>>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
>>> no?
>>
>> While it's true, you need to look into the implementation of `as_raw`.
>> It could very well return a null pointer...
>>
>> This is where we can use a `Guarantee` on that function. But since it's
>> not shorter than `.0.get()`, I would just remove it.
>
> We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
> a `Guarantee` to be clean, we should probably fix it up in a treewide change.
>
> as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
> much less obvious.

Yeah I guess then we need to do the treewide change... I don't have the
bandwidth for that, we can probably make this a good-first-issue.

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Alistair Popple]

On Fri, Jul 11, 2025 at 10:46:13PM +0200, Benno Lossin wrote:
> On Fri Jul 11, 2025 at 9:33 PM CEST, Danilo Krummrich wrote:
> > On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
> >> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> >>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
> >>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
> >>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> >>>>> index 8435f8132e38..5c35a66a5251 100644
> >>>>> --- a/rust/kernel/pci.rs
> >>>>> +++ b/rust/kernel/pci.rs
> >>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> >>>>>  
> >>>>>  impl Device {
> >>>>>      /// Returns the PCI vendor ID.
> >>>>> +    #[inline]
> >>>>>      pub fn vendor_id(&self) -> u16 {
> >>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> >>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
> >>>>
> >>>> s/by its type invariant/by the type invariants of `Self`,/
> >>>> s/always//
> >>>>
> >>>> Also, which invariant does this refer to? The only one that I can see
> >>>> is:
> >>>>
> >>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
> >>>>
> >>>> And this doesn't say anything about the validity of `self.as_raw()`...
> >>>
> >>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
> >>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
> >>> no?
> >>
> >> While it's true, you need to look into the implementation of `as_raw`.
> >> It could very well return a null pointer...
> >>
> >> This is where we can use a `Guarantee` on that function. But since it's
> >> not shorter than `.0.get()`, I would just remove it.
> >
> > We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
> > a `Guarantee` to be clean, we should probably fix it up in a treewide change.
> >
> > as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
> > much less obvious.

Coming from a C kernel programming background I agree `.as_raw()` is more
obvious than `.0.get()`. However now I'm confused ... what if anything needs
changing to get these two small patches merged?

Thanks.

 - Alistair

> Yeah I guess then we need to do the treewide change... I don't have the
> bandwidth for that, we can probably make this a good-first-issue.
> 
> ---
> Cheers,
> Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On Tue Jul 22, 2025 at 7:17 AM CEST, Alistair Popple wrote:
> On Fri, Jul 11, 2025 at 10:46:13PM +0200, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 9:33 PM CEST, Danilo Krummrich wrote:
>> > On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
>> >> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>> >>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
>> >>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>> >>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> >>>>> index 8435f8132e38..5c35a66a5251 100644
>> >>>>> --- a/rust/kernel/pci.rs
>> >>>>> +++ b/rust/kernel/pci.rs
>> >>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>> >>>>>  
>> >>>>>  impl Device {
>> >>>>>      /// Returns the PCI vendor ID.
>> >>>>> +    #[inline]
>> >>>>>      pub fn vendor_id(&self) -> u16 {
>> >>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> >>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>> >>>>
>> >>>> s/by its type invariant/by the type invariants of `Self`,/
>> >>>> s/always//
>> >>>>
>> >>>> Also, which invariant does this refer to? The only one that I can see
>> >>>> is:
>> >>>>
>> >>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>> >>>>
>> >>>> And this doesn't say anything about the validity of `self.as_raw()`...
>> >>>
>> >>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
>> >>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
>> >>> no?
>> >>
>> >> While it's true, you need to look into the implementation of `as_raw`.
>> >> It could very well return a null pointer...
>> >>
>> >> This is where we can use a `Guarantee` on that function. But since it's
>> >> not shorter than `.0.get()`, I would just remove it.
>> >
>> > We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
>> > a `Guarantee` to be clean, we should probably fix it up in a treewide change.
>> >
>> > as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
>> > much less obvious.
>
> Coming from a C kernel programming background I agree `.as_raw()` is more
> obvious than `.0.get()`. However now I'm confused ... what if anything needs
> changing to get these two small patches merged?

I think they're good, but we're pretty late in the cycle now. That should be
fine though, we can probably take them through the nova tree, or in the worst
case share a tag, if needed.

Given that, it would probably be good to add the Guarantee section on as_raw(),
as proposed by Benno, right away.

@Benno: Any proposal on what this section should say?

One minor nit would be to start the safety comments with a capital letter
instead.

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Tue Jul 22, 2025 at 7:17 AM CEST, Alistair Popple wrote:
> On Fri, Jul 11, 2025 at 10:46:13PM +0200, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 9:33 PM CEST, Danilo Krummrich wrote:
>> > On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
>> >> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>> >>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
>> >>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>> >>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> >>>>> index 8435f8132e38..5c35a66a5251 100644
>> >>>>> --- a/rust/kernel/pci.rs
>> >>>>> +++ b/rust/kernel/pci.rs
>> >>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>> >>>>>  
>> >>>>>  impl Device {
>> >>>>>      /// Returns the PCI vendor ID.
>> >>>>> +    #[inline]
>> >>>>>      pub fn vendor_id(&self) -> u16 {
>> >>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> >>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>> >>>>
>> >>>> s/by its type invariant/by the type invariants of `Self`,/
>> >>>> s/always//
>> >>>>
>> >>>> Also, which invariant does this refer to? The only one that I can see
>> >>>> is:
>> >>>>
>> >>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>> >>>>
>> >>>> And this doesn't say anything about the validity of `self.as_raw()`...
>> >>>
>> >>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
>> >>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
>> >>> no?
>> >>
>> >> While it's true, you need to look into the implementation of `as_raw`.
>> >> It could very well return a null pointer...
>> >>
>> >> This is where we can use a `Guarantee` on that function. But since it's
>> >> not shorter than `.0.get()`, I would just remove it.
>> >
>> > We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
>> > a `Guarantee` to be clean, we should probably fix it up in a treewide change.
>> >
>> > as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
>> > much less obvious.
>
> Coming from a C kernel programming background I agree `.as_raw()` is more
> obvious than `.0.get()`.

Makes sense, then I wouldn't recommend changing it.

> However now I'm confused ... what if anything needs changing to get
> these two small patches merged?

I'd like to see `as_raw` get a `Guarantee` section, but that is
independent from this patch series.

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
> I think they're good, but we're pretty late in the cycle now. That should be
> fine though, we can probably take them through the nova tree, or in the worst
> case share a tag, if needed.
>
> Given that, it would probably be good to add the Guarantee section on as_raw(),
> as proposed by Benno, right away.
>
> @Benno: Any proposal on what this section should say?

At a minimum I'd say "The returned pointer is valid.", but that doesn't
really say for what it's valid... AFAIK you're mostly using this pointer
to pass it to the C side, in that case, how about:

    /// # Guarantees
    ///
    /// The returned pointer is valid for reads and writes from the C side for as long as `self` exists.

Maybe we need to change it a bit more, but let's just start with this.

(If you're also using the pointer from Rust, then we need to make
changes)

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On 7/22/25 12:57 PM, Benno Lossin wrote:
> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
>> I think they're good, but we're pretty late in the cycle now. That should be
>> fine though, we can probably take them through the nova tree, or in the worst
>> case share a tag, if needed.
>>
>> Given that, it would probably be good to add the Guarantee section on as_raw(),
>> as proposed by Benno, right away.
>>
>> @Benno: Any proposal on what this section should say?
> 
> At a minimum I'd say "The returned pointer is valid.", but that doesn't
> really say for what it's valid... AFAIK you're mostly using this pointer
> to pass it to the C side, in that case, how about:

It is used for for FFI calls and to access fields of the underlying
struct pci_dev.

>      /// # Guarantees
>      ///
>      /// The returned pointer is valid for reads and writes from the C side for as long as `self` exists.
> 
> Maybe we need to change it a bit more, but let's just start with this.

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Tue Jul 22, 2025 at 1:02 PM CEST, Danilo Krummrich wrote:
> On 7/22/25 12:57 PM, Benno Lossin wrote:
>> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
>>> I think they're good, but we're pretty late in the cycle now. That should be
>>> fine though, we can probably take them through the nova tree, or in the worst
>>> case share a tag, if needed.
>>>
>>> Given that, it would probably be good to add the Guarantee section on as_raw(),
>>> as proposed by Benno, right away.
>>>
>>> @Benno: Any proposal on what this section should say?
>> 
>> At a minimum I'd say "The returned pointer is valid.", but that doesn't
>> really say for what it's valid... AFAIK you're mostly using this pointer
>> to pass it to the C side, in that case, how about:
>
> It is used for for FFI calls and to access fields of the underlying
> struct pci_dev.

By "access fields" you mean read-only?

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Alice Ryhl]

On Tue, Jul 22, 2025 at 12:57 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
> > I think they're good, but we're pretty late in the cycle now. That should be
> > fine though, we can probably take them through the nova tree, or in the worst
> > case share a tag, if needed.
> >
> > Given that, it would probably be good to add the Guarantee section on as_raw(),
> > as proposed by Benno, right away.
> >
> > @Benno: Any proposal on what this section should say?
>
> At a minimum I'd say "The returned pointer is valid.", but that doesn't
> really say for what it's valid... AFAIK you're mostly using this pointer
> to pass it to the C side, in that case, how about:
>
>     /// # Guarantees
>     ///
>     /// The returned pointer is valid for reads and writes from the C side for as long as `self` exists.
>
> Maybe we need to change it a bit more, but let's just start with this.
>
> (If you're also using the pointer from Rust, then we need to make
> changes)

Honestly I think this is a bit over the top. I wouldn't bother adding
a section like that to every single as_raw() method out there.

Alice

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On 7/22/25 1:21 PM, Benno Lossin wrote:
> On Tue Jul 22, 2025 at 1:02 PM CEST, Danilo Krummrich wrote:
>> On 7/22/25 12:57 PM, Benno Lossin wrote:
>>> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
>>>> I think they're good, but we're pretty late in the cycle now. That should be
>>>> fine though, we can probably take them through the nova tree, or in the worst
>>>> case share a tag, if needed.
>>>>
>>>> Given that, it would probably be good to add the Guarantee section on as_raw(),
>>>> as proposed by Benno, right away.
>>>>
>>>> @Benno: Any proposal on what this section should say?
>>>
>>> At a minimum I'd say "The returned pointer is valid.", but that doesn't
>>> really say for what it's valid... AFAIK you're mostly using this pointer
>>> to pass it to the C side, in that case, how about:
>>
>> It is used for for FFI calls and to access fields of the underlying
>> struct pci_dev.
> 
> By "access fields" you mean read-only?

We might also write them, but currently we only write them through FFI calls on
the C side.

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Tue Jul 22, 2025 at 1:35 PM CEST, Alice Ryhl wrote:
> On Tue, Jul 22, 2025 at 12:57 PM Benno Lossin <lossin@kernel.org> wrote:
>>
>> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
>> > I think they're good, but we're pretty late in the cycle now. That should be
>> > fine though, we can probably take them through the nova tree, or in the worst
>> > case share a tag, if needed.
>> >
>> > Given that, it would probably be good to add the Guarantee section on as_raw(),
>> > as proposed by Benno, right away.
>> >
>> > @Benno: Any proposal on what this section should say?
>>
>> At a minimum I'd say "The returned pointer is valid.", but that doesn't
>> really say for what it's valid... AFAIK you're mostly using this pointer
>> to pass it to the C side, in that case, how about:
>>
>>     /// # Guarantees
>>     ///
>>     /// The returned pointer is valid for reads and writes from the C side for as long as `self` exists.
>>
>> Maybe we need to change it a bit more, but let's just start with this.
>>
>> (If you're also using the pointer from Rust, then we need to make
>> changes)
>
> Honestly I think this is a bit over the top. I wouldn't bother adding
> a section like that to every single as_raw() method out there.

Hmm. And then just assume that these kinds of functions return valid
pointers? I get that this is annoying to put on every function...

Another option would be to have a `Ptr<'a, T>` type that is a valid
pointer, but doesn't allow writing/reading safely (you need to justify
why it's not a data race). And for FFI there could be an `as_ptr`
function.

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Danilo Krummrich]

On Tue Jul 22, 2025 at 2:08 PM CEST, Benno Lossin wrote:
> On Tue Jul 22, 2025 at 1:35 PM CEST, Alice Ryhl wrote:
>> On Tue, Jul 22, 2025 at 12:57 PM Benno Lossin <lossin@kernel.org> wrote:
>>>
>>> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
>>> > I think they're good, but we're pretty late in the cycle now. That should be
>>> > fine though, we can probably take them through the nova tree, or in the worst
>>> > case share a tag, if needed.
>>> >
>>> > Given that, it would probably be good to add the Guarantee section on as_raw(),
>>> > as proposed by Benno, right away.
>>> >
>>> > @Benno: Any proposal on what this section should say?
>>>
>>> At a minimum I'd say "The returned pointer is valid.", but that doesn't
>>> really say for what it's valid... AFAIK you're mostly using this pointer
>>> to pass it to the C side, in that case, how about:
>>>
>>>     /// # Guarantees
>>>     ///
>>>     /// The returned pointer is valid for reads and writes from the C side for as long as `self` exists.
>>>
>>> Maybe we need to change it a bit more, but let's just start with this.
>>>
>>> (If you're also using the pointer from Rust, then we need to make
>>> changes)
>>
>> Honestly I think this is a bit over the top. I wouldn't bother adding
>> a section like that to every single as_raw() method out there.
>
> Hmm. And then just assume that these kinds of functions return valid
> pointers? I get that this is annoying to put on every function...
>
> Another option would be to have a `Ptr<'a, T>` type that is a valid
> pointer, but doesn't allow writing/reading safely (you need to justify
> why it's not a data race). And for FFI there could be an `as_ptr`
> function.

I don't understand where's the difference between the two. For FFI calls we'd
also have to justify it's not a data race, no?

The only guarantee we take as granted from as_raw() is that it returns a raw
pointer to the wrapped FFI type in Self, i.e. it points to valid memory. Any
additional guarantees may come from the context where the pointer is used and
which specific fields it is used to access.

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Benno Lossin]

On Tue Jul 22, 2025 at 2:49 PM CEST, Danilo Krummrich wrote:
> On Tue Jul 22, 2025 at 2:08 PM CEST, Benno Lossin wrote:
>> On Tue Jul 22, 2025 at 1:35 PM CEST, Alice Ryhl wrote:
>>> On Tue, Jul 22, 2025 at 12:57 PM Benno Lossin <lossin@kernel.org> wrote:
>>>>
>>>> On Tue Jul 22, 2025 at 11:51 AM CEST, Danilo Krummrich wrote:
>>>> > I think they're good, but we're pretty late in the cycle now. That should be
>>>> > fine though, we can probably take them through the nova tree, or in the worst
>>>> > case share a tag, if needed.
>>>> >
>>>> > Given that, it would probably be good to add the Guarantee section on as_raw(),
>>>> > as proposed by Benno, right away.
>>>> >
>>>> > @Benno: Any proposal on what this section should say?
>>>>
>>>> At a minimum I'd say "The returned pointer is valid.", but that doesn't
>>>> really say for what it's valid... AFAIK you're mostly using this pointer
>>>> to pass it to the C side, in that case, how about:
>>>>
>>>>     /// # Guarantees
>>>>     ///
>>>>     /// The returned pointer is valid for reads and writes from the C side for as long as `self` exists.
>>>>
>>>> Maybe we need to change it a bit more, but let's just start with this.
>>>>
>>>> (If you're also using the pointer from Rust, then we need to make
>>>> changes)
>>>
>>> Honestly I think this is a bit over the top. I wouldn't bother adding
>>> a section like that to every single as_raw() method out there.
>>
>> Hmm. And then just assume that these kinds of functions return valid
>> pointers? I get that this is annoying to put on every function...
>>
>> Another option would be to have a `Ptr<'a, T>` type that is a valid
>> pointer, but doesn't allow writing/reading safely (you need to justify
>> why it's not a data race). And for FFI there could be an `as_ptr`
>> function.
>
> I don't understand where's the difference between the two. For FFI calls we'd
> also have to justify it's not a data race, no?

Yes, but there you need a raw pointer.

> The only guarantee we take as granted from as_raw() is that it returns a raw
> pointer to the wrapped FFI type in Self, i.e. it points to valid memory. Any
> additional guarantees may come from the context where the pointer is used and
> which specific fields it is used to access.

Sure you need additional guarantees from the context, but you also need
the fact that the pointer coming from `as_raw` isn't just a random
pointer, but that it is derived from the reference...

I don't have any good plan forward for this, so maybe we should revisit
this in the future...

---
Cheers,
Benno

Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

[Alistair Popple]

On Tue, Jul 22, 2025 at 11:51:48AM +0200, Danilo Krummrich wrote:
> On Tue Jul 22, 2025 at 7:17 AM CEST, Alistair Popple wrote:
> > On Fri, Jul 11, 2025 at 10:46:13PM +0200, Benno Lossin wrote:
> >> On Fri Jul 11, 2025 at 9:33 PM CEST, Danilo Krummrich wrote:
> >> > On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
> >> >> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> >> >>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
> >> >>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
> >> >>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> >> >>>>> index 8435f8132e38..5c35a66a5251 100644
> >> >>>>> --- a/rust/kernel/pci.rs
> >> >>>>> +++ b/rust/kernel/pci.rs
> >> >>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
> >> >>>>>  
> >> >>>>>  impl Device {
> >> >>>>>      /// Returns the PCI vendor ID.
> >> >>>>> +    #[inline]
> >> >>>>>      pub fn vendor_id(&self) -> u16 {
> >> >>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
> >> >>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
> >> >>>>
> >> >>>> s/by its type invariant/by the type invariants of `Self`,/
> >> >>>> s/always//
> >> >>>>
> >> >>>> Also, which invariant does this refer to? The only one that I can see
> >> >>>> is:
> >> >>>>
> >> >>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
> >> >>>>
> >> >>>> And this doesn't say anything about the validity of `self.as_raw()`...
> >> >>>
> >> >>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
> >> >>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
> >> >>> no?
> >> >>
> >> >> While it's true, you need to look into the implementation of `as_raw`.
> >> >> It could very well return a null pointer...
> >> >>
> >> >> This is where we can use a `Guarantee` on that function. But since it's
> >> >> not shorter than `.0.get()`, I would just remove it.
> >> >
> >> > We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
> >> > a `Guarantee` to be clean, we should probably fix it up in a treewide change.
> >> >
> >> > as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
> >> > much less obvious.
> >
> > Coming from a C kernel programming background I agree `.as_raw()` is more
> > obvious than `.0.get()`. However now I'm confused ... what if anything needs
> > changing to get these two small patches merged?
> 
> I think they're good, but we're pretty late in the cycle now. That should be
> fine though, we can probably take them through the nova tree, or in the worst
> case share a tag, if needed.

Thanks, although I don't have any burning need to get them in for this cycle.
Next cycle would be fine too, I just wanted to make progress getting them off my
TODO list and it wasn't clear if more changes were needed.

On that front is seems like the discussion has settled on maybe we need to do
something in future, but not for these patches? So aside from the minor nit
below (which I will fix) I don't know of anything else that needs changing.

> Given that, it would probably be good to add the Guarantee section on as_raw(),
> as proposed by Benno, right away.
> 
> @Benno: Any proposal on what this section should say?
> 
> One minor nit would be to start the safety comments with a capital letter
> instead.