Re: [PATCH 06/11] rust/block: Add I/O buffer traits

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]

Il mer 12 feb 2025, 18:23 Kevin Wolf <kwolf@redhat.com> ha scritto:

> Am 12.02.2025 um 17:48 hat Paolo Bonzini geschrieben:
> > On 2/11/25 22:43, Kevin Wolf wrote:
> > > +/// Implementing `SizedIoBuffer` provides an implementation for
> [`IoBuffer`] without having to
> > > +/// implement any functions manually.
> > > +///
> > > +/// # Safety
> > > +///
> > > +/// Types implementing `SizedIoBuffer` guarantee that the whole
> object can be accessed as an I/O
> > > +/// buffer that is safe to contain any byte patterns.
> > > +pub unsafe trait SizedIoBuffer: Sized {
> >
> > This is similar to the ByteValued trait in rust-vmm.  Can you name it
> > the same so that we can later consider replacing it?
>
> I'm not sure if it's the best name, but could be done, of course.
>
> Though the more interesting thing to replace it with eventually might be
> the zerocopy crate which has derive macros that check that the condition
> is actually fulfilled. I just didn't feel like bringing in new external
> dependencies in this first series.
>

Good idea though. zerocopy has no extra dependencies, and I agree that
sooner or later we're going to include it, so you might as well go for it.

The build.rs file is ludicrously overengineered, but converting it to meson
should be easy.

> > +    fn from_byte_slice(buf: &[u8]) -> Option<&Self> {
> > > +        if buf.len() < std::mem::size_of::<Self>() {
> > > +            return None;
> > > +        }
>
> This is a semantic difference compared to ByteValued::from_slice(),
> which requires the sizes to match exactly. For the probe function, I
> actually make use of the relaxed requirement here to access a header
> struct in a larger buffer passed from C.
>

Indeed it's similar but not the same. I haven't checked how you'd write it
with vm-memory (it could be hdr.as_bytes().read_obj(0), or maybe there's
something better), but it's something that could be added there too.

> If you want, the function can be written also
> >
> >     // SAFETY: implementing SizedIoBuffer promises that any byte pattern
> >     // is valid for the type
> >     match unsafe { buf.align_to::<Self>() } {
> >         ([], mid, _) => mid.get(0),
> >         _ => None
> >     }
> >
> > (trick stolen from rust-vmm, in fact).
>
> Clever way to avoid ptr::is_aligned(), but I feel a bit harder to
> understand than just open-coding it like above? (And probably less
> efficient, but I don't know how relevant that is.)
>

Probably not much and a lot of dead code elimination can happen, but either
way is fine of course.

Paolo



> Kevin
>
>

[-- Attachment #2: Type: text/html, Size: 4244 bytes --]