Re: [PATCH v4 2/2] qdisk - hw/block/xen_disk: grant copy implementation

[Anthony PERARD <anthony.perard@citrix.com>]

On Tue, Aug 09, 2016 at 07:34:14PM +0200, Paulina Szubarczyk wrote:
> On 08/09/2016 06:56 PM, Anthony PERARD wrote:
> > On Tue, Aug 02, 2016 at 04:06:30PM +0200, Paulina Szubarczyk wrote:
> > > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> > > index 640c31e..e80c61f 100644
> > > --- a/include/hw/xen/xen_common.h
> > > +++ b/include/hw/xen/xen_common.h
> > > @@ -25,6 +25,31 @@
> > >    */
> > > 
> > >   /* Xen 4.2 through 4.6 */
> > > +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 480
> > > +
> > > +struct xengnttab_grant_copy_segment {
> > > +    union xengnttab_copy_ptr {
> > > +        void *virt;
> > > +        struct {
> > > +            uint32_t ref;
> > > +            uint16_t offset;
> > > +            uint16_t domid;
> > > +        } foreign;
> > > +    } source, dest;
> > > +    uint16_t len;
> > > +    uint16_t flags;
> > > +    int16_t status;
> > > +};
> > 
> > I don't think it's a good idee to define a struct that is not going to
> > be used, and does not belong here. The typedef is OK.
> 
> I added it since it is needed to know all the fields of that struct in
> ioreq_copy but if I could replace that function with stubs I will do that.
> 
> > 
> > In xen_disk.c, you could use "#if CONFIG_XEN_CTRL_INTERFACE_VERSION ..."
> > around free_buffers, ioreq_init_copy_buffers and ioreq_copy, and replace
> > them by stubs when Xen does not support grant copy. I think that would
> > be better.
> > 
> > Also, could you try to compile again xen unstable, without your other patch?
> > Right now, it does not compile.
> 
> That is true, I am sorry. I need to add the headers that are included in the
> #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 471 */
> 
> #include <xenevtchn.h>
> #include <xengnttab.h>
> #include <xenforeignmemory.h>
> 
> #endif
> 
> May I move that part to separate #if CONFIG_XEN_CTRL_INTERFACE_VERSION >=
> 471 in that header?

You could could add your code at the end of xen_common.h instead of the
beginning, I think that would work. (At the end of xen_common.h,
xengnttab_handle should be define, either in the file or via an
include.)

> > 
> > > +typedef struct xengnttab_grant_copy_segment xengnttab_grant_copy_segment_t;
> > > +
> > > +static inline int xengnttab_grant_copy(xengnttab_handle *xgt, uint32_t count,
> > > +                                       xengnttab_grant_copy_segment_t *segs)
> > > +{
> > > +    return -1;
> > 
> > return -ENOSYS would be more appropriate.
> > 
> > 
> > Otherwise, the patch looks good.
> > 
> > Thanks,
> > 
> Thank you,
> 
> Paulina

-- 
Anthony PERARD

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