On Fri, Feb 17, 2023, Anish Moorthy wrote: > On Thu, Feb 16, 2023 at 10:53 AM Anish Moorthy <amoorthy@xxxxxxxxxx> wrote: > > > > On Wed, Feb 15, 2023 at 12:59 AM Oliver Upton <oliver.upton@xxxxxxxxx> wrote: > > > > > > How is userspace expected to differentiate the gup_fast() failed exit > > > from the guest-private memory exit? Sorry, missed this comment first time around. I don't see any reason why userspace needs to uniquely identify the gup_fast() case. Similar to page faults from hardware, KVM should describe the access in sufficient detail so that userspace can resolve the issue, whatever that may be, but KVM should make any assumptions about _why_ the failure occurred. gup_fast() failing in itself isn't interesting. The _intended_ behavior is that KVM will exit if and only if the guest accesses a valid page that hasn't yet been transfered from the source, but the _actual_ behavior is that KVM will exit if the page isn't faulted in for _any_ reason. Even tagging the access NOWAIT is speculative to some degree, as that suggests the access may have succeeded if KVM "waited", which may or may not be true. E.g. see the virtiofs trunctionation use case that planted the original seed for this approach: https://lore.kernel.org/kvm/20201005161620.GC11938@xxxxxxxxxxxxxxx. > > > I don't think flags are a good idea for this, as it comes with the > > > illusion that both events can happen on a single exit. In reality, these > > > are mutually exclusive. They aren't mutually exclusive. Obviously KVM will only detect one other the other, but it's entirely possible that a guest could be accessing the "wrong" flavor of a page _and_ for that flavor to not be faulted in. A smart userspace should see that (a) it needs to change the memory attributes and (b) that it needs to demand the to-be-installed page from the source. > > > A fault type/code would be better here, with the option to add flags at > > > a later date that could be used to further describe the exit (if > > > needed). > > > > Agreed. Not agreed :-) > > + struct { > > + __u32 fault_code; > > + __u64 reserved; > > + __u64 gpa; > > + __u64 size; > > + } memory_fault; > > > > The "reserved" field is meant to be the placeholder for a future "flags" field. > > Let me know if there's a better/more conventional way to achieve this. > > On Thu, Feb 16, 2023 at 10:53 AM Anish Moorthy <amoorthy@xxxxxxxxxx> wrote: > > > > 1. As Oliver touches on earlier, we'll probably want to use this same field for > > different classes of memory fault in the future (such as the ones which Chao > > is introducing in [1]): so it does make sense to add "code" and "flags" > > fields which can be used to communicate more information to the user (and > > which can just be set to MEM_FAULT_NOWAIT/0 in this series). > > Let me walk back my responses here: I took a closer look at Chao's series, and > it doesn't seem that I should be trying to share KVM_EXIT_MEMORY_FAULT with it > in the first place. As far as I can understand (not that much, to be clear :) > we're signaling unrelated things, so it makes more sense to use different exits > (ie, rename mine to KVM_EXIT_MEMORY_FAULT_NOWAIT). That would prevent any > potential confusion about mutual exclusivity. Hard "no" on a separate exit reason unless someone comes up with a very compelling argument. Chao's UPM series is not yet merged, i.e. is not set in stone. If the proposed functionality in Chao's series is lacking and/or will conflict with this UFFD, then we can and should address those issues _before_ it gets merged.
