On Tuesday, November 29, 2016 03:09:31 AM Paolo Bonzini wrote:
>
> > > >> Let's separate the three conditions (R/W/X):
> > > >>
> > > >> if ((error_code & PFERR_FETCH_MASK) {
> > > >> if ((spte & (shadow_x_mask|shadow_nx_mask))
> > > >> == shadow_x_mask) {
> > > >> fault_handled = true;
> > > >> break;
> > > >> }
> > > >> }
> > > >> if (error_code & PFERR_WRITE_MASK) {
> > > >> if (is_writable_pte(spte)) {
> > > >> fault_handled = true;
> > > >> break;
> > > >> }
> > > >> remove_write_prot =
> > > >> spte_can_locklessly_be_made_writable(spte);
> > > >> }
> > > >> if (!(error_code & PFERR_PRESENT_MASK)) {
> > > >> if (!is_access_track_spte(spte)) {
> > > >> fault_handled = true;
> > > >> break;
> > > >> }
> > > >> remove_acc_track = true;
> > > >> }
> > > >
> > > > I think the third block is incorrect e.g. it will set fault_handled =
> > > > true even
> > > > for a completely zero PTE.
> > >
> > > A completely zero PTE would have been filtered before by the
> > > is_shadow_present_pte check, wouldn't it?
> >
> > Oh, the is_shadow_present_pte check was actually removed in the patch. We could
> > add it back, minus the ret = true statement, and then it would filter the zero
> > PTE case. But I still think that the other form:
> >
> > if ((error_code & PFERR_USER_MASK) &&
> > (spte & PT_PRESENT_MASK)) {
> > fault_handled = true;
> > break;
> > }
> >
> > is simpler as it is directly analogous to the cases for fetch and write.
> > Please let me know if you think otherwise.
>
> Fair enough, but add a comment to explain the error_code check because I
> don't get it. :)
The error_code check verifies that it was a Read access, as PFERR_USER_MASK
is mapped to EPT_VIOLATION_READ. However, I have just realized that this isn’t
the case when not using EPT. So I’ll just use the following instead, which
works for both EPT and non-EPT:
if (error_code & PFERR_FETCH_MASK) {
if ((spte & (shadow_x_mask | shadow_nx_mask))
== shadow_x_mask) {
fault_handled = true;
break;
}
} else if (error_code & PFERR_WRITE_MASK) {
if (is_writable_pte(spte)) {
fault_handled = true;
break;
}
remove_write_prot =
spte_can_locklessly_be_made_writable(spte);
} else {
/* Fault was on Read access */
if (spte & PT_PRESENT_MASK) {
fault_handled = true;
break;
}
}
remove_acc_track = is_access_track_spte(spte);
Thanks,
Junaid
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
