Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT

[Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>]

On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 19.07.18 at 10:43, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> > On 07/19/2018 11:30 AM, Jan Beulich wrote:
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > On 19.07.18 at 10:18, <aisaila@xxxxxxxxxxxxxxx> wrote:
> > > > On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
> > > > > 
> > > > > > 
> > > > > > On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitde
> > > > > > fender.c 
> > > > > > +            break;
> > > > > > +        case p2m_access_x:
> > > > > > +            flags &= ~_PAGE_RW;
> > > > > > +            break;
> > > > > > +        case p2m_access_rwx:
> > > > > > +        default:
> > > > > > +            break;
> > > > > >     }
> > > > > I think you want another blank line here too.
> > > > > 
> > > > > Also, this doesn’t seem to capture the ‘r’ part of the
> > > > > equation —
> > > > > shouldn’t p2m_access_n end up with a not-present p2m entry?
> > > > SVM dosen't explicitly provide a read access bit so we treat
> > > > read and
> > > > write the same way.
> > > Read and write can't possibly be treated the same. You ought to
> > > use
> > > the present bit to deny read (really: any) access, as also
> > > implied by
> > > George's response.
> > They aren't treated the same as far sending out a vm_event goes.
> > However, if we understand this correctly, there is no way to cause
> > only
> > read, or only write exits for NPT. They are bundled together under
> > _PAGE_RW.
> > 
> > So svm_do_nested_pgfault() tries to sort these out:
> > 
> > 1781     struct npfec npfec = {
> > 1782         .read_access = !(pfec & PFEC_insn_fetch),
> > 1783         .write_access = !!(pfec & PFEC_write_access),
> > 1784         .insn_fetch = !!(pfec & PFEC_insn_fetch),
> > 1785         .present = !!(pfec & PFEC_page_present),
> > 1786     };
> > 1787
> > 1788     /* These bits are mutually exclusive */
> > 1789     if ( pfec & NPT_PFEC_with_gla )
> > 1790         npfec.kind = npfec_kind_with_gla;
> > 1791     else if ( pfec & NPT_PFEC_in_gpt )
> > 1792         npfec.kind = npfec_kind_in_gpt;
> > 1793
> > 1794     ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
> > 
> > but a read access is considered to be something that's not an insn
> > fetch, and we only have a specific bit set for the write.
> > 
> > Since hvm_hap_nested_page_fault() looks at npfec to decide when to
> > send
> > out a vm_event, this takes care of handling reads and writes
> > differently
> > at this level; however it's not possible to set separate single
> > "don't
> > read" or "don't write" exit-causing flags with NPT.
> All fine, but George's question was raised in the context of
> permission
> conversion from p2m to pte representation.

I've tried a test with xen access that sets XENMEM_access_n on all the
pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
the p2m_access_n case and the guest fails with triple fault. There are
a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
invalid if the flag is not present. I don't think this is the way to go
with the p2m_access_n flags.

Thanks, 
Alex

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel