Re: [Qemu-devel] [PATCH 06/18] xen: add grant table interface for XenDevice-s

[Paul Durrant]

> -----Original Message-----
> From: Anthony PERARD [mailto:address@hidden
> Sent: 03 December 2018 15:46
> To: Paul Durrant <address@hidden>
> Cc: address@hidden; address@hidden; xen-
> address@hidden; Stefano Stabellini <address@hidden>
> Subject: Re: [PATCH 06/18] xen: add grant table interface for XenDevice-s
> 
> On Wed, Nov 21, 2018 at 03:11:59PM +0000, Paul Durrant wrote:
> > The legacy PV backend infrastructure provides functions to map, unmap
> and
> > copy pages granted by frontends. Similar functionality will be required
> > by XenDevice implementations so this patch adds the necessary support.
> >
> > Signed-off-by: Paul Durrant <address@hidden>
> > ---
> > +typedef struct XenDeviceGrantCopySegment {
> > +    union {
> > +        void *virt;
> > +        struct {
> > +            uint32_t ref;
> > +            off_t offset;
> > +        } foreign;
> > +    } source, dest;
> 
> Why is there a union between `source` and `dest`, I don't see any way
> (another field) to distinguish which is which. Can't we have a
> segment without `source`/`dest`?

How? I think introducing two separate segment definitions depending on whether 
it is a copy-to-guest or copy-from-guest is a little ugly.

> It mimic the
> xengnttab_grant_copy_segment_t but that doesn't seems very useful as it
> doesn't really prevent mistake.

It mimics that struct apart from the field static which direction the transfer 
is because, to maintain compatibility, the entire copy goes only one way or the 
other.

> 
> > +    size_t len;
> > +} XenDeviceGrantCopySegment;
> 
> Anyway, it's not very important:
> 
> Reviewed-by: Anthony PERARD <address@hidden>

Thanks.

  Paul

> 
> Thanks,
> 
> --
> Anthony PERARD