Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm

["Yu, Zhang" <yu.c.zhang@xxxxxxxxxxxxxxx>]

On 11/28/2014 5:57 PM, Jan Beulich wrote:
> > > > On 28.11.14 at 08:59, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2838,7 +2838,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
> > gla,
> >        * to the mmio handler.
> >        */
> >       if ( (p2mt == p2m_mmio_dm) ||
> > -         (npfec.write_access && (p2mt == p2m_ram_ro)) )
> > +         (npfec.write_access &&
> > +               ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )
> Why are you corrupting indentation here?
Thanks for your comments, Jan.
The indentation here is to make sure the
((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm)) are grouped 
together. But I am not sure if this is correct according to xen coding 
style. I may have misunderstood your previous comments on Sep 3, which 
said "the indentation would need adjustment" in reply to "[Xen-devel] 
[PATCH v3 1/2] x86: add p2m_mmio_write_dm"
> Furthermore the code you modify here suggests that p2m_ram_ro
> already has the needed semantics - writes get passed to the DM.
> None of the other changes you make, and none of the other uses
> of p2m_ram_ro appear to be in conflict with your intentions, so
> you'd really need to explain better why you need the new type.
Thanks Jan.
To my understanding, pages with p2m_ram_ro are not supposed to be 
modified by guest. So in __hvm_copy(), when p2m type of a page is 
p2m_ram_rom, no copy will occur.
However, to our usage, we just wanna this page to be write protected, so 
that our device model can be triggered to do some emulation. The content 
written to this page is supposed not to be dropped. This way, if 
sequentially a read operation is performed by guest to this page, the 
guest will still see its anticipated value.
Maybe I need to update my commit message to explain this more clearly. :)

> > @@ -5922,6 +5923,8 @@ long do_hvm_op(unsigned long op, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >                   a.mem_type =  HVMMEM_ram_rw;
> >               else if ( p2m_is_grant(t) )
> >                   a.mem_type =  HVMMEM_ram_rw;
> > +            else if ( t == p2m_mmio_write_dm )
> > +                a.mem_type = HVMMEM_mmio_write_dm;
> >               else
> >                   a.mem_type =  HVMMEM_mmio_dm;
> >               rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> > @@ -5941,7 +5944,8 @@ long do_hvm_op(unsigned long op, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >           static const p2m_type_t memtype[] = {
> >               [HVMMEM_ram_rw]  = p2m_ram_rw,
> >               [HVMMEM_ram_ro]  = p2m_ram_ro,
> > -            [HVMMEM_mmio_dm] = p2m_mmio_dm
> > +            [HVMMEM_mmio_dm] = p2m_mmio_dm,
> > +            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
> >           };
> >
> >           if ( copy_from_guest(&a, arg, 1) )
> > @@ -5987,14 +5991,17 @@ long do_hvm_op(unsigned long op, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >                   rc = -EAGAIN;
> >                   goto param_fail4;
> >               }
> > +
> Stray addition of a blank line?
>
> >               if ( !p2m_is_ram(t) &&
> > -                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
> > +                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
> > +                 t != p2m_mmio_write_dm )
> Do you really want to permit e.g. transitions between mmio_dm and
> mmio_write_dm? We should be as restrictive as possible here to not
> open up paths to security problems.
>
> >               {
> >                   put_gfn(d, pfn);
> >                   goto param_fail4;
> >               }
> >
> >               rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
> > +
> >               put_gfn(d, pfn);
> >               if ( rc )
> >                   goto param_fail4;
> Another stray newline addition.
>
> > --- a/xen/include/asm-x86/p2m.h
> > +++ b/xen/include/asm-x86/p2m.h
> > @@ -72,6 +72,7 @@ typedef enum {
> >       p2m_ram_shared = 12,          /* Shared or sharable memory */
> >       p2m_ram_broken = 13,          /* Broken page, access cause domain crash */
> >       p2m_map_foreign  = 14,        /* ram pages from foreign domain */
> > +    p2m_mmio_write_dm = 15,       /* Read-only; writes go to the device model 
> > */
> >   } p2m_type_t;
> >
> >   /* Modifiers to the query */
> If the new type is really needed, shouldn't this get added to
> P2M_RO_TYPES?
Well, previsouly, I wished to differenciate the HVMMEM_ram_ro and the 
newly added HVMMEM_mmio_write_dm in the 
HVMOP_get_mem_type/HVMOP_set_mem_type hypercall. I'd rather think of 
this new type as write protected than read only.
But I'll take a look, to see if this can be added to P2M_RO_TYPES. Thanks.

Yu

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