Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

virtio_queue: Add descriptors_utils.rs #278

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

MatiasVara
Copy link
Contributor

@MatiasVara MatiasVara commented Nov 15, 2023

Summary of the PR

This commit adds descriptors_utils.rs from virtiofsd (daa4e32). This module provides the Reader/Writer traits to iterate over descriptors. Devices shall rely on this interface instead of manipulate descriptors. The reason to this is that the distribution is driver-dependent, i.e., virtio does not force any particular way to distribute the data over descriptors. The version in this commit removes read_to_at() and write_from_at() functions that are required only for virtiofsd. This commit is on top of 0.9 tag. To try this module, I am adding support for it in the virtio-sound device at https://github.com/MatiasVara/vhost-device/tree/use-descriptor-utils-virtio-sound.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@MatiasVara
Copy link
Contributor Author

@germag feel free to comment.

@Ablu
Copy link
Contributor

Ablu commented Nov 15, 2023

@MatiasVara thanks a lot for updating this! This is something which we really need. I will review this tomorrow in more detail!

Just a couple of minor comments before diving into a review:

  • Would be great to add a reference from which virtiofsd version this was adapted
  • Has someone done a comparison whether there have been new changes on this over on the crosvm project?

@MatiasVara
Copy link
Contributor Author

@MatiasVara thanks a lot for updating this! This is something which we really need. I will review this tomorrow in more detail!

Just a couple of minor comments before diving into a review:

  • Would be great to add a reference from which virtiofsd version this was adapted
  • Has someone done a comparison whether there have been new changes on this over on the crosvm project?

I missed those two points from our previous discussion. I will add a reference to the virtiofsd version. In regards to the second point, I just did a quick review of the crosvm version and found that it doesn't require a lifetime parameter when instantiated. However, I did not go any further. The differences may need to be better understood.

@stsquad
Copy link
Collaborator

stsquad commented Nov 15, 2023

Is virtiofsd's read/write_at not likely to be useful for other backends? Is it a very special case only virtiofsd benefits from?

Copy link
Contributor

@Ablu Ablu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some random comments, but nothing extraordinary. One thing I wonder about is whether the separation into Reader and Writer makes a lot of sense for the user.

Does a user of this really care that much about it? I wonder whether it could not be a single object and the API could be used something like:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

(maybe overly simplified, but should show the idea)

With the proposed API here that has to be done manually like writter_status.split_at(size_of::<VirtioSoundHeader>()). While this certainly bring great improvements for the user, I wonder whether the ergonomics could be even better with a single object? 🤔 Sure, some special case may require "seeking" in the chain, but I guess 90%+ of the uses will just be sequential processing/parsing followed by writing the answer?

crates/virtio-queue/src/descriptor_utils.rs Outdated Show resolved Hide resolved
crates/virtio-queue/src/descriptor_utils.rs Outdated Show resolved Hide resolved
// writes are happening.
unsafe {
copy_nonoverlapping(rem.as_ptr(), vs.ptr_guard_mut().as_ptr(), copy_len);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slice.read_volatile(vs) here :)

crates/virtio-queue/src/descriptor_utils.rs Outdated Show resolved Hide resolved
crates/virtio-queue/src/descriptor_utils.rs Outdated Show resolved Hide resolved
crates/virtio-queue/src/descriptor_utils.rs Outdated Show resolved Hide resolved
crates/virtio-queue/src/descriptor_utils.rs Outdated Show resolved Hide resolved
@MatiasVara
Copy link
Contributor Author

Is virtiofsd's read/write_at not likely to be useful for other backends? Is it a very special case only virtiofsd benefits from?

Probably, I can add them if they are required for other backends. If not, I believe each device can "adapt or extend" current traits to suit its particular needs.

@MatiasVara
Copy link
Contributor Author

@Ablu Thanks for the review! I will try to answer some of the comments during today.

Some random comments, but nothing extraordinary. One thing I wonder about is whether the separation into Reader and Writer makes a lot of sense for the user.

Does a user of this really care that much about it? I wonder whether it could not be a single object and the API could be used something like:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

(maybe overly simplified, but should show the idea)

With the proposed API here that has to be done manually like writter_status.split_at(size_of::<VirtioSoundHeader>()). While this certainly bring great improvements for the user, I wonder whether the ergonomics could be even better with a single object? 🤔 Sure, some special case may require "seeking" in the chain, but I guess 90%+ of the uses will just be sequential processing/parsing followed by writing the answer?

Having a single object would simplify things. My guess about this design is that the chain of descriptors could be divided into two areas: a chain with read-only descriptors followed by a chain with write-only descriptors. This is the only requirement from the virtio-spec about the descriptors distribution (2.6.4.2 in Virtio Spec v1.1). As far as I understand it, when a Reader and Writer are instantiated, they "point" to the beginning of the area they correspond to. A single object shall automatically take into account such an offset. This is not a problem at all, and such a logic should be added to write/read_obj().

@Ablu
Copy link
Contributor

Ablu commented Nov 16, 2023

Having a single object would simplify things. My guess about this design is that the chain of descriptors could be divided into two areas: a chain with read-only descriptors followed by a chain with write-only descriptors. This is the only requirement from the virtio-spec about the descriptors distribution (2.6.4.2 in Virtio Spec v1.1). As far as I understand it, when a Reader and Writer are instantiated, they "point" to the beginning of the area they correspond to. A single object shall automatically take into account such an offset. This is not a problem at all, and such a logic should be added to write/read_obj().

Ah. Did not realize that readable first, writeable after that even was a global rule. Not sure whether we need specific logic to verify this (they flags on the descriptor sound sufficient to me). But it confirms that read, then write is the only case :).

@MatiasVara: Want to try to adapt it to a single object? Eventually, I think we should target for this API being the default way to work with descriptors. So probably the descriptor chain iterator on the queue should in the end just return something that implements this high-level API :).

@MatiasVara
Copy link
Contributor Author

Having a single object would simplify things. My guess about this design is that the chain of descriptors could be divided into two areas: a chain with read-only descriptors followed by a chain with write-only descriptors. This is the only requirement from the virtio-spec about the descriptors distribution (2.6.4.2 in Virtio Spec v1.1). As far as I understand it, when a Reader and Writer are instantiated, they "point" to the beginning of the area they correspond to. A single object shall automatically take into account such an offset. This is not a problem at all, and such a logic should be added to write/read_obj().

Ah. Did not realize that readable first, writeable after that even was a global rule. Not sure whether we need specific logic to verify this (they flags on the descriptor sound sufficient to me). But it confirms that read, then write is the only case :).

@MatiasVara: Want to try to adapt it to a single object? Eventually, I think we should target for this API being the default way to work with descriptors. So probably the descriptor chain iterator on the queue should in the end just return something that implements this high-level API :).

Yes, I can try to adapt it to a single object.

@MatiasVara
Copy link
Contributor Author

Some random comments, but nothing extraordinary. One thing I wonder about is whether the separation into Reader and Writer makes a lot of sense for the user.

Does a user of this really care that much about it? I wonder whether it could not be a single object and the API could be used something like:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

(maybe overly simplified, but should show the idea)

With the proposed API here that has to be done manually like writter_status.split_at(size_of::<VirtioSoundHeader>()). While this certainly bring great improvements for the user, I wonder whether the ergonomics could be even better with a single object? 🤔 Sure, some special case may require "seeking" in the chain, but I guess 90%+ of the uses will just be sequential processing/parsing followed by writing the answer?

Would it make sense an interface like this? Note that this is just a draft and names will be different:

pub struct Walker<'a>{
    r: Reader<'a>,
    w: Writer<'a>,
    payload: Writer<'a>
}

impl <'a> Walker<'a>{
    pub fn new<M>(
        mem: &'a GuestMemoryMmap,
        desc_chain: DescriptorChain<M>,
        offset: usize
    ) -> Result<Walker<'a>>
    where
       M: Deref,
       M: Clone,
       M::Target: GuestMemory + Sized,
    {
        let reader = Reader::new(mem, desc_chain.clone()).unwrap();
        let mut writer = Writer::new(mem, desc_chain.clone()).unwrap();
        let payload = writer.split_at(offset).unwrap();
        Ok(Walker {
            r: reader,
            w: writer,
            payload
        })
    }

    pub fn read_obj<T: ByteValued>(&mut self) -> io::Result<T> {
        self.r.read_obj()
    }
    
    pub fn write_obj<T: ByteValued>(&mut self, val: T) -> io::Result<()> {
        self.w.write_obj(val)
    }

    pub fn write_payload_obj<T: ByteValued>(&mut self, val: T) -> io::Result<()> {
        self.payload.write_obj(val)
    }
}

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

@stefano-garzarella
Copy link
Member

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Nov 17, 2023

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

I used to use the split_at() function on the writer to get a new writer that begins in the offset in which the payload of the request starts. In the case of virtio-sound, on write-only descriptors, there is first the status and then the payload but we first write the payload and then the status. I would require a sort of seek() to first write the payload and then the status (or just change the way we currently do it). Since all requests have a status, I proposed to split_at when Walker is instantiated and then provide a method to explicitly write in the area that corresponds with the payload which is after the status header.

@Ablu
Copy link
Contributor

Ablu commented Nov 17, 2023

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Nov 17, 2023

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

Could you elaborate?, Thanks.

@stefano-garzarella
Copy link
Member

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

I used to use the split_at() function on the writer to get a new writer that begins in the offset in which the payload of the request starts. In the case of virtio-sound, on write-only descriptors, there is first the status and then the payload but we first write the payload and then the status. I would require a sort of seek() to first write the payload and then the status (or just change the way we currently do it). Since all requests have a status, I proposed to split_at when Walker is instantiated and then provide a method to explicitly write in the area that corresponds with the payload which is after the status header.

mmm, payload it looks like too specific for virtio-sound use case, for example in virtio-blk is it the opposite (payload is before the status).
What about having a vector of Writers/Readers, then using an index?

@Ablu
Copy link
Contributor

Ablu commented Nov 17, 2023

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

Could you elaborate?, Thanks.

Sorry for not being super clear... What I meant was I see no pressing need for having Writer and Reader types for walking the descriptor chain.

Going back to what I sketched before:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

Here, descriptor_chain could directly be what this PR currently calls DescriptorChainConsumer (probably merged into or containing the current desc_chain type). Then read_obj()/write_obj could be defined on that while keeping some internal cursor.

I am not exactly sure whether I understand why the payload differentiation is needed though. Why can't I just do:

desc_chain.read_obj::<SomeHeader>()?;
desc_chain.write_obj(payload)?;
desc_chain.write_obj(status)?;

🤔

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Nov 20, 2023

I am not exactly sure whether I understand why the payload differentiation is needed though. Why can't I just do:

desc_chain.read_obj::<SomeHeader>()?;
desc_chain.write_obj(payload)?;
desc_chain.write_obj(status)?;

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

I used to use the split_at() function on the writer to get a new writer that begins in the offset in which the payload of the request starts. In the case of virtio-sound, on write-only descriptors, there is first the status and then the payload but we first write the payload and then the status. I would require a sort of seek() to first write the payload and then the status (or just change the way we currently do it). Since all requests have a status, I proposed to split_at when Walker is instantiated and then provide a method to explicitly write in the area that corresponds with the payload which is after the status header.

mmm, payload it looks like too specific for virtio-sound use case, for example in virtio-blk is it the opposite (payload is before the status). What about having a vector of Writers/Readers, then using an index?

Could you elaborate? I proposed payload to work around split_at() which is not simply to implement if both Reader and Writer are in the same object. I think split_at() is useful because it allows you to write to the write-only descriptor chain in a independent ways. For example, you can have on Writer for the payload and another for the status. Otherwise, you should require a sort of seek() before to write in the correct position.

@stefano-garzarella
Copy link
Member

mmm, payload it looks like too specific for virtio-sound use case, for example in virtio-blk is it the opposite (payload is before the status). What about having a vector of Writers/Readers, then using an index?

Could you elaborate? I proposed payload to work around split_at() which is not simply to implement if both Reader and Writer are in the same object.

Yeah, but this is meaningful only for virtio-sound, because for example for virito-blk the payload is before the status.

For some others maybe payload doesn't mean much, so IMHO we should provide more generic API. What I was suggesting was for example provide a vector of offsets to the constructor, where to split the Writers or Readers, then use the index in the API to write a specific section we split.

I think split_at() is useful because it allows you write to the write-only descriptor chain in a independent way.

If we want a single Walker, then maybe we need to provide methods to split the Walker, but I don't think we should split into header and payload in it, because that's really device-specific and the user has to do the split.

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Nov 20, 2023

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

Could you elaborate?, Thanks.

Sorry for not being super clear... What I meant was I see no pressing need for having Writer and Reader types for walking the descriptor chain.

Going back to what I sketched before:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

I think you can't use the same desc_chain for reading and writing but I may be wrong. I think a single object would still have two desc_chain one for reading and another for writing. In such a case, split_at may be hard to implement.

Here, descriptor_chain could directly be what this PR currently calls DescriptorChainConsumer (probably merged into or containing the current desc_chain type). Then read_obj()/write_obj could be defined on that while keeping some internal cursor.

I am not exactly sure whether I understand why the payload differentiation is needed though. Why can't I just do:

desc_chain.read_obj::<SomeHeader>()?;
desc_chain.write_obj(payload)?;
desc_chain.write_obj(status)?;

🤔

I used payload to work around split_at(). The status is not always at the end of the chain. In that case, you would need a sort of seek before the write_obj(). I used split_at() instead so I can write the payload and the status independently.

@Ablu
Copy link
Contributor

Ablu commented Nov 20, 2023

I used payload to work around split_at(). The status is not always at the end of the chain. In that case, you would need a sort of seek before the write_obj(). I used split_at() instead so I can write the payload and the status independently.

I wonder: Couldn't one just .clone() at the appropriate state in order to detach one reader/writer instance from another?

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Nov 20, 2023

I used payload to work around split_at(). The status is not always at the end of the chain. In that case, you would need a sort of seek before the write_obj(). I used split_at() instead so I can write the payload and the status independently.

I wonder: Couldn't one just .clone() at the appropriate state in order to detach one reader/writer instance from another?

I think the example could be rewritten as the following draft:

desc_chain_reader.read_obj::<SomeHeader>()?;
desc_chain_writer.write_obj(payload)?;
desc_chain_writer_status = desc_chain_writer.clone();
desc_chain_writer_status.seek(sizeof(payload)); // note that seek() is not currently implemented
desc_chain_writer.write_obj(status)?;

I think it is still required to separate the desc_chain for reading and for writing. I am not sure if the use of such a clone should require the use of Arc() or Mutex(). Also, seek() is not currently implemented. The same using split_at() would be (it is a draft):

reader.read_obj::<SomeHeader>()?;
writer_status = writer_content.split_at(sizeof(payload))
writer_status.write_obj(status)?;

This allows users to easily share writer_content and writer_status. For example, writer_content may be used as a parameter to a function that processes the content, etc. In the case of a single object, if seek() is not provided, a split() would result in an object that would still contain a reference to a reader/writer that is not needed any more.

I think we have to modify current implementation only if changes improve the way it is currently used. It is also true that those changes may raise after some utilization in the future. The other way to think would be to let each device add descriptor_utils.rs (as it is done for virtiofsd) and modify it as needed. In the future, we can move descriptor_utils.rs to virtio-queue crate when it is more clear how other devices use it. It is not clear to me how to modify Reader/Writer in a single object in a way that produces any benefits. I am not saying that that does not exist. A third option would be to provide something much more simpler, e.g., what we proposed for virtio-sound, in which some API simply allows reading and writing over descriptors in a continuous way so the device is allowed to support any distribution. This would require rewriting descriptor_utils from scratch, which is not the goal of this PR.    

@Ablu
Copy link
Contributor

Ablu commented Nov 20, 2023

I think it is still required to separate the desc_chain for reading and for writing.

Why is that? 🤔 Chances are that I misunderstand something, but I have not found the reason spelled out so far.

I think we have to modify current implementation only if changes improve the way it is currently used. It is also true that those changes may raise after some utilization in the future. The other way to think would be to let each device add descriptor_utils.rs (as it is done for virtiofsd) and modify it as needed. In the future, we can move descriptor_utils.rs to virtio-queue crate when it is more clear how other devices use it. It is not clear to me how to modify Reader/Writer in a single object in a way that produces any benefits. I am not saying that that does not exist. A third option would be to provide something much more simpler, e.g., what we proposed for virtio-sound, in which some API simply allows reading and writing over descriptors in a continuous way so the device is allowed to support any distribution. This would require rewriting descriptor_utils from scratch, which is not the goal of this PR.

I think unless we have some upstream solution, people probably won't put the brain cycles into creating a one-fits-all solutions that suits everyone equally well. I was hoping that one could turn it into a simpler to use API with just a few tweaks. I will try to play with this somewhen during this week and see how it feels.

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Nov 20, 2023

I think it is still required to separate the desc_chain for reading and for writing.

Why is that? 🤔 Chances are that I misunderstand something, but I have not found the reason spelled out so far.

I did not explain myself very well, I meant that Reader relies on desc_chain.readable() whereas Writer relies on desc_chain.writable().

@MatiasVara
Copy link
Contributor Author

MatiasVara commented Jan 3, 2024

cargo llvm-cov --html

Thanks, I spotted some functions and corner cases that were not covered. Some of them are:

  • split() for writer
  • flush()

I will add tests for them.

@MatiasVara MatiasVara force-pushed the add-descriptor-utils branch 5 times, most recently from 8467762 to efc1885 Compare January 4, 2024 20:59
@stefano-garzarella
Copy link
Member

Except for the license (which as I've already written I think is a pre-existing problem to be discussed separately), the rest seems to me to be in very good shape and I will try to use it in vhost-device-vsock soon.

@MatiasVara thanks for this effort!

@Ablu
Copy link
Contributor

Ablu commented Jan 5, 2024

Thanks again for working on this! This will greatly simplify descriptor walking and fix a lot of bugs in current code :)

@stefano-garzarella
Copy link
Member

I was trying to use Reader and Writer in https://github.com/rust-vmm/vm-virtio/blob/main/virtio-vsock/src/packet.rs, but I needed it to accept a generic GuestMemory, so I gave it a try and after struggling with it, I found a few things that seem to work:

stefano-garzarella@98497a0

@Ablu @MatiasVara What do you think?

@Ablu
Copy link
Contributor

Ablu commented Jan 11, 2024

@Ablu @MatiasVara What do you think?

Ah, I tried the same before but missed being able to set the lifetime via WithBitmapSlice. That certainly fixes the problems that I had where region.find_region() would result in something tied to a lifetime to the region (and thus not being able to add it to the Writer instance).

So it looks good to me. Yet, this interface in vm-memory is just way too complex to use without loosing sanity. I also dislike the Deref use in DescriptorChain. It disallows properly using lifetimes through it. IMHO it should just be a &GuestMemory there. But I failed refactoring this in sensible steps as everything is so intertwined.

@stefano-garzarella
Copy link
Member

@Ablu @MatiasVara What do you think?

Ah, I tried the same before but missed being able to set the lifetime via WithBitmapSlice. That certainly fixes the problems that I had where region.find_region() would result in something tied to a lifetime to the region (and thus not being able to add it to the Writer instance).

Yeah, I was inspired by virtio-vsock API:

pub fn from_tx_virtq_chain<M, T>(

BTW, in the end I think I'll change the virtio-vsock API to accept Reader and Writer from the caller, so I'm not sure if I really need the changes I proposed. But if you think they can improve a bit this PR, feel free to incorporate.

So it looks good to me. Yet, this interface in vm-memory is just way too complex to use without loosing sanity. I also dislike the Deref use in DescriptorChain. It disallows properly using lifetimes through it. IMHO it should just be a &GuestMemory there. But I failed refactoring this in sensible steps as everything is so intertwined

Okay, so I'm not the only one who almost went crazy with it....

@MatiasVara
Copy link
Contributor Author

I was trying to use Reader and Writer in https://github.com/rust-vmm/vm-virtio/blob/main/virtio-vsock/src/packet.rs, but I needed it to accept a generic GuestMemory, so I gave it a try and after struggling with it, I found a few things that seem to work:

stefano-garzarella@98497a0

@Ablu @MatiasVara What do you think?

I think is it is a good idea. I just pushed a version with the changes.

Copy link
Collaborator

@slp slp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

This commit adds descriptors_utils.rs from virtiofsd. This module
provides the Reader/Writer classes to iterate over descriptors. Devices
shall rely on this interface instead of manipulate descriptors. The
version in this commit removes read_to_at() and write_from_at() that are
required only for virtiofsd. To instantiate these helpers, user shall
use the reader()/writer() functions defined in the context of
DescriptorChain.

Co-developed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
@stefano-garzarella
Copy link
Member

I'm going to merge this, since we want to use it in vhost-devices.
We have 3 acks, so it should be okay.

@stefano-garzarella stefano-garzarella merged commit d949c0b into rust-vmm:main Feb 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants