Re: [Xen-devel] [PATCH for-next 06/16] xen/arm: Extend copy_to_guest to support copying from/to guest physical address

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 8 Dec 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/12/17 23:01, Stefano Stabellini wrote:
> > On Wed, 6 Dec 2017, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 12/06/2017 01:22 AM, Stefano Stabellini wrote:
> > > > On Thu, 23 Nov 2017, Julien Grall wrote:
> > > > > The only differences between copy_to_guest and
> > > > > access_guest_memory_by_ipa
> > > > > are:
> > > > >       - The latter does not support copying data crossing page
> > > > > boundary
> > > > >       - The former is copying from/to guest VA whilst the latter from
> > > > >       guest PA
> > > > > 
> > > > > copy_to_guest can easily be extended to support copying from/to guest
> > > > > physical address. For that a new bit is used to tell whether linear
> > > > > address or ipa is been used.
> > > > > 
> > > > > Lastly access_guest_memory_by_ipa is reimplemented using
> > > > > copy_to_guest.
> > > > > This also has the benefits to extend the use of it, it is now possible
> > > > > to copy data crossing page boundary.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > > > 
> > > > Ah! This is the reason why previous patches were not using vaddr_t. It
> > > > makes sense now. May I suggest we use something different from paddr_t
> > > > in copy_guest for addr type? I don't think is correct to specify addr as
> > > > paddr_t when it could be vaddr_t; in the future we could have type
> > > > checks on them.
> > > > 
> > > > I suggest we specify it as u64, but if you have a better idea go for it.
> > > 
> > > We should not use more u64 in the code. uint64_t could be a solution but
> > > even
> > > that, I don't see the reason. How are you sure the physical address will
> > > always fit in 64-bit?
> > > 
> > > On the other side, very likely vaddr_t will fit in paddr_t. So paddr_t is
> > > the
> > > right way to go for me.
> > 
> > What about introducing xaddr_t?
> 
> I would prefer uint64_t in that case. xaddr_t is quite confusing to read and
> could be misused.
> 
> > Or at least:
> > 
> >    static struct page_info *translate_get_page(struct vcpu *v, paddr_t /*or
> > vaddr_t */ addr
> 
> I can do that as well. What's your preference?

I prefer uint64_t

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel