Re: [Xen-devel] [PATCH v3 2/2] ioreq-server: write protected range and forwarding

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

> -----Original Message-----
> From: Paul Durrant [mailto:Paul.Durrant@xxxxxxxxxx]
> Sent: Friday, September 5, 2014 5:02 PM
> To: Ye, Wei; Tian, Kevin; xen-devel@xxxxxxxxxxxxx
> Cc: JBeulich@xxxxxxxx; Ian Campbell; Ian Jackson; Stefano Stabellini; Dugger,
> Donald D; Zhang, Yang Z; Lv, Zhiyuan; konrad.wilk@xxxxxxxxxx; Keir (Xen.org);
> Tim (Xen.org)
> Subject: RE: [PATCH v3 2/2] ioreq-server: write protected range and
> forwarding
> 
> > -----Original Message-----
> > From: Ye, Wei [mailto:wei.ye@xxxxxxxxx]
> > Sent: 05 September 2014 01:44
> > To: Kevin Tian; Paul Durrant; xen-devel@xxxxxxxxxxxxx
> > Cc: JBeulich@xxxxxxxx; Ian Campbell; Ian Jackson; Stefano Stabellini;
> > Dugger, Donald D; Zhang, Yang Z; Lv, Zhiyuan; konrad.wilk@xxxxxxxxxx;
> > Keir (Xen.org); Tim (Xen.org)
> > Subject: RE: [PATCH v3 2/2] ioreq-server: write protected range and
> > forwarding
> >
> >
> >
> > > -----Original Message-----
> > > From: Tian, Kevin
> > > Sent: Friday, September 5, 2014 7:11 AM
> > > To: Paul Durrant; Ye, Wei; xen-devel@xxxxxxxxxxxxx
> > > Cc: JBeulich@xxxxxxxx; Ian Campbell; Ian Jackson; Stefano
> > > Stabellini;
> > Dugger,
> > > Donald D; Zhang, Yang Z; Lv, Zhiyuan; konrad.wilk@xxxxxxxxxx; Keir
> > (Xen.org);
> > > Tim (Xen.org)
> > > Subject: RE: [PATCH v3 2/2] ioreq-server: write protected range and
> > > forwarding
> > >
> > > > From: Paul Durrant [mailto:Paul.Durrant@xxxxxxxxxx]
> > > > Sent: Wednesday, September 03, 2014 3:12 AM
> > > >
> > > > > -----Original Message-----
> > > > > From: Wei Ye [mailto:wei.ye@xxxxxxxxx]
> > > > > Sent: 03 September 2014 22:53
> > > > > To: xen-devel@xxxxxxxxxxxxx
> > > > > Cc: JBeulich@xxxxxxxx; Ian Campbell; Paul Durrant; Ian Jackson;
> > > > > Stefano Stabellini; donald.d.dugger@xxxxxxxxx; Kevin Tian;
> > > > > yang.z.zhang@xxxxxxxxx; zhiyuan.lv@xxxxxxxxx;
> > > > > konrad.wilk@xxxxxxxxxx; Keir (Xen.org); Tim (Xen.org); Wei Ye
> > > > > Subject: [PATCH v3 2/2] ioreq-server: write protected range and
> > > > > forwarding
> > > > >
> > > > > Extend the ioreq-server to write protect pages and forward the
> > > > > write access to the corresponding device model.
> > > > >
> > > >
> > > > Using rangesets did make the patch somewhat smaller then :-)
> > > >
> > > > > Signed-off-by: Wei Ye <wei.ye@xxxxxxxxx>
> > > > > ---
> > > > >  tools/libxc/xc_domain.c          |   33
> > > > +++++++++++++++++++++++++++
> > > > >  tools/libxc/xenctrl.h            |   18 +++++++++++++++
> > > > >  xen/arch/x86/hvm/hvm.c           |   46
> > > > > ++++++++++++++++++++++++++++++++++++++
> > > > >  xen/include/asm-x86/hvm/domain.h |    2 +-
> > > > >  xen/include/public/hvm/hvm_op.h  |    1 +
> > > > >  5 files changed, 99 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > > > > index
> > > > > 37ed141..34e6309 100644
> > > > > --- a/tools/libxc/xc_domain.c
> > > > > +++ b/tools/libxc/xc_domain.c
> > > > > @@ -1485,6 +1485,39 @@ int
> > > > > xc_hvm_unmap_pcidev_from_ioreq_server(xc_interface *xch,
> > domid_t
> > > > > domid,
> > > > >      return rc;
> > > > >  }
> > > > >
> > > > > +int xc_hvm_map_wp_range_to_ioreq_server(xc_interface *xch,
> > > domid_t
> > > > > domid,
> > > > > +                                        ioservid_t id, uint16_t set,
> > > > > +                                        uint64_t start,
> > > > > + uint64_t
> > > > end)
> > > > > +{
> > > >
> > > > Given what I said about the notion of MMIO ranges that need read
> > > > emulation but no write emulation, and the ranges you're concerned
> > > > about which require write emulation but no read emulation, I
> > > > wonder whether this could collapse into the existing
> > > > xc_hvm_map_io_range_to_ioreq_server() by adding an extra
> parameter
> > > to say whether, for a given range, you want read emulation, write
> > > > emulation or both.
> > >
> > > Agree. Add a parameter to indicate r/w permission seems cleaner.
> > >
> > > >
> > > > > +    DECLARE_HYPERCALL;
> > > > > +    DECLARE_HYPERCALL_BUFFER(xen_hvm_io_range_t, arg);
> > > > > +    int rc = -1;
> > > > > +
> > > > > +    if ( arg == NULL )
> > > > > +    {
> > > > > +        errno = -EFAULT;
> > > > > +        return -1;
> > > > > +    }
> > > > > +
> > > > > +    hypercall.op = __HYPERVISOR_hvm_op;
> > > > > +    if ( set )
> > > > > +        hypercall.arg[0] = HVMOP_map_io_range_to_ioreq_server;
> > > > > +    else
> > > > > +        hypercall.arg[0] =
> HVMOP_unmap_io_range_from_ioreq_server;
> > > > > +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> > > > > +
> > > > > +    arg->domid = domid;
> > > > > +    arg->id = id;
> > > > > +    arg->type = HVMOP_IO_RANGE_WP;
> > > > > +    arg->start = start;
> > > > > +    arg->end = end;
> > > > > +
> > > > > +    rc = do_xen_hypercall(xch, &hypercall);
> > > > > +
> > > > > +    xc_hypercall_buffer_free(xch, arg);
> > > > > +    return rc;
> > > > > +}
> > > > > +
> > > > >  int xc_hvm_destroy_ioreq_server(xc_interface *xch,
> > > > >                                  domid_t domid,
> > > > >                                  ioservid_t id) diff --git
> > > > > a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h index
> > > > > b55d857..b261602 100644
> > > > > --- a/tools/libxc/xenctrl.h
> > > > > +++ b/tools/libxc/xenctrl.h
> > > > > @@ -1943,6 +1943,24 @@ int
> > > > > xc_hvm_unmap_pcidev_from_ioreq_server(xc_interface *xch,
> > > > >                                            uint8_t function);
> > > > >
> > > > >  /**
> > > > > + * This function registers/deregisters a set of pages to be
> > > > > + write
> > > protected.
> > > > > + *
> > > > > + * @parm xch a handle to an open hypervisor interface.
> > > > > + * @parm domid the domain id to be serviced
> > > > > + * @parm id the IOREQ Server id.
> > > > > + * @parm set whether registers or deregisters: 1 for register,
> > > > > + 0 for
> > > > > deregister
> > > > > + * @parm start start of range
> > > > > + * @parm end end of range (inclusive).
> > > > > + * @return 0 on success, -1 on failure.
> > > > > + */
> > > > > +int xc_hvm_map_wp_range_to_ioreq_server(xc_interface *xch,
> > > > > +                                        domid_t domid,
> > > > > +                                        ioservid_t id,
> > > > > +                                        uint16_t set,
> > > > > +                                        uint64_t start,
> > > > > +                                        uint64_t end);
> > > > > +
> > > > > +/**
> > > > >   * This function destroys an IOREQ Server.
> > > > >   *
> > > > >   * @parm xch a handle to an open hypervisor interface.
> > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > > > > index
> > > > > adc0f93..f8c4db8 100644
> > > > > --- a/xen/arch/x86/hvm/hvm.c
> > > > > +++ b/xen/arch/x86/hvm/hvm.c
> > > > > @@ -853,6 +853,7 @@ static int
> > > > > hvm_ioreq_server_alloc_rangesets(struct
> > > > > hvm_ioreq_server *s,
> > > > >                        (i == HVMOP_IO_RANGE_PORT) ? "port" :
> > > > >                        (i == HVMOP_IO_RANGE_MEMORY) ?
> > > > "memory" :
> > > > >                        (i == HVMOP_IO_RANGE_PCI) ? "pci" :
> > > > > +                      (i == HVMOP_IO_RANGE_WP) ? "write
> > > > protection" :
> > > >
> > > > Continuing along my train of thought from above, perhaps the
> > > > rangesets should now be
> > > >
> > > > HVMOP_IO_RANGE_PORT_READ
> > > > HMVOP_IO_RANGE_PORT_WRITE
> > > > HVMOP_IO_RANGE_MEMORY_READ
> > > > HVMOP_IO_RANGE_MEMORY_WRITE
> > > > HVMOP_IO_RANGE_PCI
> > >
> > > this looks cleaner. Once we start considering the permission, WP
> > > becomes only an attribute which can be attached to either port or
> memory resource.
> > > So better not define a new type here.
> > >
> >
> > Shall we also need a type of HVMOP_IO_RANGE_MEMORY_BOTH?
> > Otherwise, if want to set
> > both r/w emulation, user needs to call the interface
> > xc_hvm_map_io_range_to_ioreq_server() twice. One is with the
> parameter
> > HVMOP_IO_RANGE_MEMORY_READ and the other with parameter
> > HVMOP_IO_RANGE_MEMORY_WRITE.
> >
> 
> That's true, so messing with the types directly is probably not the correct
> answer. Instead, break direct association between type and rangeset index
> and then hvmop_map_io_range_to_ioreq_server() can call
> hvm_map_io_range_to_ioreq_server() multiple times if necessary.
> You could declare the array ins struct hvm_ioreq_server as:
> 
> struct rangeset       *range[NR_IO_RANGE_TYPES * 2];
> 
> and then calculate index as something like:
> 
> if (flags & READ)
>   index = type;
> 
> if (flags & WRITE)
>   index = type + NR_IO_RANGE_TYPES;
> 
> Yes, it does mean double the number of rangesets if you want both read and
> write, but that's a very small amount of extra memory. If necessary it could
> be avoided by having a third set of rangesets for READ|WRITE but it would
> mean potentially having to do two lookups in hvm_select_ioreq_server().
> 
>   Paul
> 
I'm a little bit confused that like write protection for a true mmio range. If 
the true
mmio range is readable but write is porected, then where is the data can be 
read from
If the read is not forwarded to device model. Current write protection for 
normal pages
treat write as mmio is a special case. For read, data comes from the normal 
page, but
wirte go to device model for emulation. And, we also can write directly to the
normal page but forward read to device model. So I think the write protection or
read protection is meaningfull only for normal page protection, not for port or 
mmio resources.
So, double the rangeset that split READ|WRITE sets for existing resource is not 
very reasonable,  am I right?

Wei


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