Re: [Xen-devel] [PATCH v1 1/2] x86: add p2m_ram_wp

["Ye, Wei" <wei.ye@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, July 28, 2014 4:32 PM
> To: Ye, Wei
> Cc: ian.campbell@xxxxxxxxxx; Paul.Durrant@xxxxxxxxxx;
> ian.jackson@xxxxxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxx; keir@xxxxxxx; Tim Deegan
> Subject: Re: [PATCH v1 1/2] x86: add p2m_ram_wp
> 
> >>> On 28.07.14 at 19:55, <wei.ye@xxxxxxxxx> wrote:
> >  xen/arch/x86/hvm/hvm.c    |    8 +++++++-
> >  xen/arch/x86/mm/p2m-ept.c |    1 +
> >  xen/include/asm-x86/p2m.h |    8 +++++++-
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> 
> This can't be complete: At the very least p2m-pt.c also needs a change similar
> to the one to p2m-ept.c.

Yes, got it.

> 
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2738,7 +2738,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> >       * If this GFN is emulated MMIO or marked as read-only, pass the fault
> >       * to the mmio handler.
> >       */
> > -    if ( (p2mt == p2m_mmio_dm) ||
> > +    if ( (p2mt == p2m_mmio_dm) ||
> > +         (p2mt == p2m_ram_wp) ||
> >           (access_w && (p2mt == p2m_ram_ro)) )
> 
> Comparing your change with the surrounding existing code, you
> would pass even reads happening to fault (perhaps for another
> reason) to the DM, other than done for p2m_ram_ro. I don't think
> that's correct.
> 

Yes, should also access_w like the p2m_ram_ro.

> > @@ -3829,6 +3830,11 @@ static enum hvm_copy_result __hvm_copy(
> >              put_page(page);
> >              return HVMCOPY_unhandleable;
> >          }
> > +        if ( p2m_is_wp_ram(p2mt) )
> > +        {
> > +            put_page(page);
> > +            return HVMCOPY_bad_gfn_to_mfn;
> > +        }
> 
> And again this change can't be complete: __hvm_clear() would also
> need a similar change.
> 

Got it and thanks.

> > @@ -167,6 +169,9 @@ typedef unsigned int p2m_query_t;
> >   * and must not be touched. */
> >  #define P2M_BROKEN_TYPES (p2m_to_mask(p2m_ram_broken))
> >
> > +/* Write protection types */
> > +#define P2M_WP_TYPES (p2m_to_mask(p2m_ram_wp))
> > +
> >  /* Useful predicates */
> >  #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
> >  #define p2m_is_hole(_t) (p2m_to_mask(_t) & P2M_HOLE_TYPES)
> > @@ -191,6 +196,7 @@ typedef unsigned int p2m_query_t;
> >  #define p2m_is_any_ram(_t)  (p2m_to_mask(_t) &                   \
> >                               (P2M_RAM_TYPES | P2M_GRANT_TYPES |  \
> >                                p2m_to_mask(p2m_map_foreign)))
> > +#define p2m_is_wp_ram(_t)   (p2m_to_mask(_t) & P2M_WP_TYPES)
> 
> To me such single-type classes don't seem very useful. Agreed
> there are a number of pre-existing ones, but for classes where
> future extensions are rather hard to imagine I wouldn't needlessly
> add classes right away. But Tim (whom you failed to Cc anyway)
> will have the final say here.
> 

How about using the existing p2m_mmio_dm, in that way, both read & write access 
will go to device model for emulation.

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel