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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Paul Durrant [mailto:Paul.Durrant@xxxxxxxxxx]
> Sent: Thursday, September 11, 2014 7:39 AM
> 
> > -----Original Message-----
> > From: Tian, Kevin [mailto:kevin.tian@xxxxxxxxx]
> > Sent: 10 September 2014 14:02
> > To: Ye, Wei; 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
> >
> > > From: Ye, Wei
> > > Sent: Tuesday, September 09, 2014 11:10 PM
> > > > > > > > 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
> >
> > Ideally there could be such usage of a special device assignment style. For
> > example, have all the writes from the VM forwarded to an agent in Dom0,
> so
> > writes can be logged and replayed for introspection or high availability
> > purpose,
> > while having most reads from device directly for performance reason if no
> > side-effect.
> >
> > However it's only an ideal case, so likely the change we made now is
> > incomplete
> > and then anyway more changes are required to make it complete when the
> > ideal case becomes real. Regarding to that, maybe it's better to just
> > introduce
> > a new type, specifically for this write-protected memory page usage:
> >
> > >>HVMOP_IO_RANGE_WP_MEMORY
> >
> > It has effect only on normal memory (add a check on that), w/o impact on
> > existing interfaces.
> >
> > Paul, how about your thoughts?
> >
> 
> I really don't see it as a special case. You can either emulate read, write or
> both. Clearly if you're not emulating both then there has to be a page in the
> p2m so that the non-emulated case can be handled. Current MMIO is simply
> the emulate-both case for a range which has no memory backing it. If we get
> the new additional code right then it would be possible for an emulator
> registering an MMIO range to add pages into the guest for than range, and
> then only emulate read or write accesses if it doesn't need to handle both.
> 

so let's still pursue this interface change like you suggested. However because
Wei left Intel, Yu will take this work and please bear some delay for him to 
ramp
up. 

Thanks
Kevin

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