Re: [PATCH v3 00/21] xen: simplify frontend side ring setup

[Boris Ostrovsky]

On 5/5/22 4:16 AM, Juergen Gross wrote:

Many Xen PV frontends share similar code for setting up a ring page
(allocating and granting access for the backend) and for tearing it
down.

Create new service functions doing all needed steps in one go.

This requires all frontends to use a common value for an invalid
grant reference in order to make the functions idempotent.

Changes in V3:
- new patches 1 and 2, comments addressed

Changes in V2:
- new patch 9 and related changes in patches 10-18



For the patches that I was explicitly copied on:


Reviewed-by: Boris Ostrovsky 

Re: [PATCH 0/5] xen: cleanup detection of non-essential pv devices

[Boris Ostrovsky]

On 11/22/21 3:20 AM, Juergen Gross wrote:

On 22.10.21 08:47, Juergen Gross wrote:

Today the non-essential pv devices are hard coded in the xenbus driver
and this list is lacking multiple entries.

This series reworks the detection logic of non-essential devices by
adding a flag for that purpose to struct xenbus_driver.

Juergen Gross (5):
   xen: add "not_essential" flag to struct xenbus_driver
   xen: flag xen_drm_front to be not essential for system boot
   xen: flag hvc_xen to be not essential for system boot
   xen: flag pvcalls-front to be not essential for system boot
   xen: flag xen_snd_front to be not essential for system boot

  drivers/gpu/drm/xen/xen_drm_front.c    |  1 +
  drivers/input/misc/xen-kbdfront.c  |  1 +
  drivers/tty/hvc/hvc_xen.c  |  1 +
  drivers/video/fbdev/xen-fbfront.c  |  1 +
  drivers/xen/pvcalls-front.c    |  1 +
  drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
  include/xen/xenbus.h   |  1 +
  sound/xen/xen_snd_front.c  |  1 +
  8 files changed, 10 insertions(+), 11 deletions(-)







Applied to for-linus-5.16c


-boris

Re: [PATCH 0/5] xen: cleanup detection of non-essential pv devices

[Boris Ostrovsky]

On 11/22/21 3:20 AM, Juergen Gross wrote:

On 22.10.21 08:47, Juergen Gross wrote:

Today the non-essential pv devices are hard coded in the xenbus driver
and this list is lacking multiple entries.

This series reworks the detection logic of non-essential devices by
adding a flag for that purpose to struct xenbus_driver.

Juergen Gross (5):
   xen: add "not_essential" flag to struct xenbus_driver
   xen: flag xen_drm_front to be not essential for system boot
   xen: flag hvc_xen to be not essential for system boot
   xen: flag pvcalls-front to be not essential for system boot
   xen: flag xen_snd_front to be not essential for system boot

  drivers/gpu/drm/xen/xen_drm_front.c    |  1 +
  drivers/input/misc/xen-kbdfront.c  |  1 +
  drivers/tty/hvc/hvc_xen.c  |  1 +
  drivers/video/fbdev/xen-fbfront.c  |  1 +
  drivers/xen/pvcalls-front.c    |  1 +
  drivers/xen/xenbus/xenbus_probe_frontend.c | 14 +++---
  include/xen/xenbus.h   |  1 +
  sound/xen/xen_snd_front.c  |  1 +
  8 files changed, 10 insertions(+), 11 deletions(-)



Any further comments?



Reviewed-by: Boris Ostrovsky 


(I'll fix the semicolon typo in the last patch, no need to resend)

Re: [PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays

[Boris Ostrovsky]

On 2/19/21 3:32 PM, Konrad Rzeszutek Wilk wrote:
> On Sun, Feb 07, 2021 at 04:56:01PM +0100, Christoph Hellwig wrote:
>> On Thu, Feb 04, 2021 at 09:40:23AM +0100, Christoph Hellwig wrote:
>>> So one thing that has been on my mind for a while:  I'd really like
>>> to kill the separate dma ops in Xen swiotlb.  If we compare xen-swiotlb
>>> to swiotlb the main difference seems to be:
>>>
>>>  - additional reasons to bounce I/O vs the plain DMA capable
>>>  - the possibility to do a hypercall on arm/arm64
>>>  - an extra translation layer before doing the phys_to_dma and vice
>>>versa
>>>  - an special memory allocator
>>>
>>> I wonder if inbetween a few jump labels or other no overhead enablement
>>> options and possibly better use of the dma_range_map we could kill
>>> off most of swiotlb-xen instead of maintaining all this code duplication?
>> So I looked at this a bit more.
>>
>> For x86 with XENFEAT_auto_translated_physmap (how common is that?)
> Juergen, Boris please correct me if I am wrong, but that 
> XENFEAT_auto_translated_physmap
> only works for PVH guests?


That's both HVM and PVH (for dom0 it's only PVH).


-boris



>
>> pfn_to_gfn is a nop, so plain phys_to_dma/dma_to_phys do work as-is.
>>
>> xen_arch_need_swiotlb always returns true for x86, and
>> range_straddles_page_boundary should never be true for the
>> XENFEAT_auto_translated_physmap case.
> Correct. The kernel should have no clue of what the real MFNs are
> for PFNs.
>> So as far as I can tell the mapping fast path for the
>> XENFEAT_auto_translated_physmap can be trivially reused from swiotlb.
>>
>> That leaves us with the next more complicated case, x86 or fully cache
>> coherent arm{,64} without XENFEAT_auto_translated_physmap.  In that case
>> we need to patch in a phys_to_dma/dma_to_phys that performs the MFN
>> lookup, which could be done using alternatives or jump labels.
>> I think if that is done right we should also be able to let that cover
>> the foreign pages in is_xen_swiotlb_buffer/is_swiotlb_buffer, but
>> in that worst case that would need another alternative / jump label.
>>
>> For non-coherent arm{,64} we'd also need to use alternatives or jump
>> labels to for the cache maintainance ops, but that isn't a hard problem
>> either.
>>
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [patch 27/30] xen/events: Only force affinity mask for percpu interrupts

[boris . ostrovsky]

On 12/11/20 7:37 AM, Thomas Gleixner wrote:
> On Fri, Dec 11 2020 at 13:10, Jürgen Groß wrote:
>> On 11.12.20 00:20, boris.ostrov...@oracle.com wrote:
>>> On 12/10/20 2:26 PM, Thomas Gleixner wrote:
 All event channel setups bind the interrupt on CPU0 or the target CPU for
 percpu interrupts and overwrite the affinity mask with the corresponding
 cpumask. That does not make sense.

 The XEN implementation of irqchip::irq_set_affinity() already picks a
 single target CPU out of the affinity mask and the actual target is stored
 in the effective CPU mask, so destroying the user chosen affinity mask
 which might contain more than one CPU is wrong.

 Change the implementation so that the channel is bound to CPU0 at the XEN
 level and leave the affinity mask alone. At startup of the interrupt
 affinity will be assigned out of the affinity mask and the XEN binding will
 be updated.
>>>
>>> If that's the case then I wonder whether we need this call at all and 
>>> instead bind at startup time.
>> After some discussion with Thomas on IRC and xen-devel archaeology the
>> result is: this will be needed especially for systems running on a
>> single vcpu (e.g. small guests), as the .irq_set_affinity() callback
>> won't be called in this case when starting the irq.


On UP are we not then going to end up with an empty affinity mask? Or are we 
guaranteed to have it set to 1 by interrupt generic code?


This is actually why I brought this up in the first place --- a potential 
mismatch between the affinity mask and Xen-specific data (e.g. info->cpu and 
then protocol-specific data in event channel code). Even if they are 
re-synchronized later, at startup time (for SMP).


I don't see anything that would cause a problem right now but I worry that this 
inconsistency may come up at some point.


-boris


> That's right, but not limited to ARM. The same problem exists on x86 UP.
> So yes, the call makes sense, but the changelog is not really useful.
> Let me add a comment to this.
>
> Thanks,
>
> tglx
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [patch 27/30] xen/events: Only force affinity mask for percpu interrupts

[boris . ostrovsky]

On 12/10/20 2:26 PM, Thomas Gleixner wrote:
> All event channel setups bind the interrupt on CPU0 or the target CPU for
> percpu interrupts and overwrite the affinity mask with the corresponding
> cpumask. That does not make sense.
>
> The XEN implementation of irqchip::irq_set_affinity() already picks a
> single target CPU out of the affinity mask and the actual target is stored
> in the effective CPU mask, so destroying the user chosen affinity mask
> which might contain more than one CPU is wrong.
>
> Change the implementation so that the channel is bound to CPU0 at the XEN
> level and leave the affinity mask alone. At startup of the interrupt
> affinity will be assigned out of the affinity mask and the XEN binding will
> be updated. 


If that's the case then I wonder whether we need this call at all and instead 
bind at startup time.


-boris


> Only keep the enforcement for real percpu interrupts.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [patch 24/30] xen/events: Remove unused bind_evtchn_to_irq_lateeoi()

[boris . ostrovsky]

On 12/10/20 2:26 PM, Thomas Gleixner wrote:
> Signed-off-by: Thomas Gleixner 
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Cc: Stefano Stabellini 
> Cc: xen-de...@lists.xenproject.org
> ---
>  drivers/xen/events/events_base.c |6 --
>  1 file changed, 6 deletions(-)
>
> --- a/drivers/xen/events/events_base.c
> +++ b/drivers/xen/events/events_base.c
> @@ -1132,12 +1132,6 @@ int bind_evtchn_to_irq(evtchn_port_t evt
>  }
>  EXPORT_SYMBOL_GPL(bind_evtchn_to_irq);
>  
> -int bind_evtchn_to_irq_lateeoi(evtchn_port_t evtchn)
> -{
> - return bind_evtchn_to_irq_chip(evtchn, _lateeoi_chip);
> -}
> -EXPORT_SYMBOL_GPL(bind_evtchn_to_irq_lateeoi);



include/xen/events.h also needs to be updated (and in the next patch for 
xen_set_affinity_evtchn() as well).


-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/11] x86/xen: open code alloc_vm_area in arch_gnttab_valloc

[boris . ostrovsky]

On 9/24/20 9:58 AM, Christoph Hellwig wrote:
> Replace the last call to alloc_vm_area with an open coded version using
> an iterator in struct gnttab_vm_area instead of the triple indirection
> magic in alloc_vm_area.
>
> Signed-off-by: Christoph Hellwig 


Reviewed-by: Boris Ostrovsky 


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/11] xen/xenbus: use apply_to_page_range directly in xenbus_map_ring_pv

[boris . ostrovsky]

On 9/24/20 9:58 AM, Christoph Hellwig wrote:
> Replacing alloc_vm_area with get_vm_area_caller + apply_page_range
> allows to fill put the phys_addr values directly instead of doing
> another loop over all addresses.
>
> Signed-off-by: Christoph Hellwig 


Reviewed-by: Boris Ostrovsky 


-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] x86/xen: open code alloc_vm_area in arch_gnttab_valloc

[boris . ostrovsky]

On 9/22/20 11:27 AM, Christoph Hellwig wrote:
> On Tue, Sep 22, 2020 at 11:24:20AM -0400, boris.ostrov...@oracle.com wrote:
>> On 9/22/20 10:58 AM, Christoph Hellwig wrote:
>>> On Mon, Sep 21, 2020 at 04:44:10PM -0400, boris.ostrov...@oracle.com wrote:
 This will end up incrementing area->ptes pointer. So perhaps something like


 pte_t **ptes = area->ptes;

 if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
     PAGE_SIZE * nr_frames, gnttab_apply, )) {

    ...
>>> Yeah.  What do you think of this version? 
>>
>> Oh yes, this is way better. This now can actually be read without trying to 
>> mentally unwind triple pointers. (You probably want to initialize idx to 
>> zero before calling apply_to_page_range(), I am not sure it's guaranteed to 
>> be zero).
> Both instances are static variables, thus in .bss and initialized.
> So unless you insist I don't think we need a manual one.


Yes, you are right. (I thought perhaps this code could be called more than once 
but no, it can't).


-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] x86/xen: open code alloc_vm_area in arch_gnttab_valloc

[boris . ostrovsky]

On 9/22/20 10:58 AM, Christoph Hellwig wrote:
> On Mon, Sep 21, 2020 at 04:44:10PM -0400, boris.ostrov...@oracle.com wrote:
>> This will end up incrementing area->ptes pointer. So perhaps something like
>>
>>
>> pte_t **ptes = area->ptes;
>>
>> if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
>>     PAGE_SIZE * nr_frames, gnttab_apply, )) {
>>
>>    ...
> Yeah.  What do you think of this version? 


Oh yes, this is way better. This now can actually be read without trying to 
mentally unwind triple pointers. (You probably want to initialize idx to zero 
before calling apply_to_page_range(), I am not sure it's guaranteed to be zero).


>  I think it is a little
> cleaner and matches what xenbus does.  At this point it probably should
> be split into a Xen and a alloc_vm_area removal patch, though.


Right.


-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] x86/xen: open code alloc_vm_area in arch_gnttab_valloc

[boris . ostrovsky]

On 9/18/20 12:37 PM, Christoph Hellwig wrote:
>  
> +static int gnttab_apply(pte_t *pte, unsigned long addr, void *data)
> +{
> + pte_t ***p = data;
> +
> + **p = pte;
> + (*p)++;
> + return 0;
> +}
> +
>  static int arch_gnttab_valloc(struct gnttab_vm_area *area, unsigned 
> nr_frames)
>  {
>   area->ptes = kmalloc_array(nr_frames, sizeof(*area->ptes), GFP_KERNEL);
>   if (area->ptes == NULL)
>   return -ENOMEM;
> -
> - area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes);
> - if (area->area == NULL) {
> - kfree(area->ptes);
> - return -ENOMEM;
> - }
> -
> + area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_IOREMAP);
> + if (!area->area)
> + goto out_free_ptes;
> + if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
> + PAGE_SIZE * nr_frames, gnttab_apply, >ptes))


This will end up incrementing area->ptes pointer. So perhaps something like


pte_t **ptes = area->ptes;

if (apply_to_page_range(_mm, (unsigned long)area->area->addr,
    PAGE_SIZE * nr_frames, gnttab_apply, )) {

   ...

}


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] xen: add helpers to allocate unpopulated memory

[Boris Ostrovsky]

On 7/24/20 10:34 AM, David Hildenbrand wrote:
> CCing Dan
>
> On 24.07.20 14:42, Roger Pau Monne wrote:
>> diff --git a/drivers/xen/unpopulated-alloc.c 
>> b/drivers/xen/unpopulated-alloc.c
>> new file mode 100644
>> index ..aaa91cefbbf9
>> --- /dev/null
>> +++ b/drivers/xen/unpopulated-alloc.c
>> @@ -0,0 +1,222 @@



>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +
>> +static DEFINE_MUTEX(lock);
>> +static LIST_HEAD(list);
>> +static unsigned int count;
>> +
>> +static int fill(unsigned int nr_pages)


Less generic names? How about  list_lock, pg_list, pg_count,
fill_pglist()? (But these are bad too, so maybe you can come up with
something better)


>> +{
>> +struct dev_pagemap *pgmap;
>> +void *vaddr;
>> +unsigned int i, alloc_pages = round_up(nr_pages, PAGES_PER_SECTION);
>> +int nid, ret;
>> +
>> +pgmap = kzalloc(sizeof(*pgmap), GFP_KERNEL);
>> +if (!pgmap)
>> +return -ENOMEM;
>> +
>> +pgmap->type = MEMORY_DEVICE_DEVDAX;
>> +pgmap->res.name = "XEN SCRATCH";


Typically iomem resources only capitalize first letters.


>> +pgmap->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>> +
>> +ret = allocate_resource(_resource, >res,
>> +alloc_pages * PAGE_SIZE, 0, -1,
>> +PAGES_PER_SECTION * PAGE_SIZE, NULL, NULL);


Are we not going to end up with a whole bunch of "Xen scratch" resource
ranges for each miss in the page list? Or do we expect them to get merged?


>> +if (ret < 0) {
>> +pr_err("Cannot allocate new IOMEM resource\n");
>> +kfree(pgmap);
>> +return ret;
>> +}
>> +
>> +nid = memory_add_physaddr_to_nid(pgmap->res.start);


Should we consider page range crossing node boundaries?


>> +
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +/*
>> + * We don't support PV MMU when Linux and Xen is using
>> + * different page granularity.
>> + */
>> +BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>> +
>> +/*
>> + * memremap will build page tables for the new memory so
>> + * the p2m must contain invalid entries so the correct
>> + * non-present PTEs will be written.
>> + *
>> + * If a failure occurs, the original (identity) p2m entries
>> + * are not restored since this region is now known not to
>> + * conflict with any devices.
>> + */
>> +if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +xen_pfn_t pfn = PFN_DOWN(pgmap->res.start);
>> +
>> +for (i = 0; i < alloc_pages; i++) {
>> +if (!set_phys_to_machine(pfn + i, INVALID_P2M_ENTRY)) {
>> +pr_warn("set_phys_to_machine() failed, no 
>> memory added\n");
>> +release_resource(>res);
>> +kfree(pgmap);
>> +return -ENOMEM;
>> +}
>> +}
>> +}
>> +#endif
>> +
>> +vaddr = memremap_pages(pgmap, nid);
>> +if (IS_ERR(vaddr)) {
>> +pr_err("Cannot remap memory range\n");
>> +release_resource(>res);
>> +kfree(pgmap);
>> +return PTR_ERR(vaddr);
>> +}
>> +
>> +for (i = 0; i < alloc_pages; i++) {
>> +struct page *pg = virt_to_page(vaddr + PAGE_SIZE * i);
>> +
>> +BUG_ON(!virt_addr_valid(vaddr + PAGE_SIZE * i));
>> +list_add(>lru, );
>> +count++;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/**
>> + * xen_alloc_unpopulated_pages - alloc unpopulated pages
>> + * @nr_pages: Number of pages
>> + * @pages: pages returned
>> + * @return 0 on success, error otherwise
>> + */
>> +int xen_alloc_unpopulated_pages(unsigned int nr_pages, struct page **pages)
>> +{
>> +unsigned int i;
>> +int ret = 0;
>> +
>> +mutex_lock();
>> +if (count < nr_pages) {
>> +ret = fill(nr_pages);


(nr_pages - count) ?


>> +if (ret)
>> +goto out;
>> +}
>> +
>> +for (i = 0; i < nr_pages; i++) {
>> +struct page *pg = list_first_entry_or_null(, struct page,
>> +   lru);
>> +
>> +BUG_ON(!pg);
>> +list_del(>lru);
>> +count--;
>> +pages[i] = pg;
>> +
>> +#ifdef CONFIG_XEN_HAVE_PVMMU
>> +/*
>> + * We don't support PV MMU when Linux and Xen is using
>> + * different page granularity.
>> + */
>> +BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>> +
>> +if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> +ret = xen_alloc_p2m_entry(page_to_pfn(pg));
>> +if (ret < 0) {
>> +unsigned int j;
>> +
>> +for (j = 0; j <= i; j++) {
>> 

Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

[Boris Ostrovsky]

On 11/7/19 3:36 PM, Jason Gunthorpe wrote:
> On Tue, Nov 05, 2019 at 10:16:46AM -0500, Boris Ostrovsky wrote:
>
>>> So, I suppose it can be relaxed to a null test and a WARN_ON that it
>>> hasn't changed?
>> You mean
>>
>> if (use_ptemod) {
>>     WARN_ON(map->vma != vma);
>>     ...
>>
>>
>> Yes, that sounds good.
> I amended my copy of the patch with the above, has this rework shown
> signs of working?

Yes, it works fine.

But please don't forget notifier ops initialization.

With those two changes,

Reviewed-by: Boris Ostrovsky 

>
> @@ -436,7 +436,8 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
> struct gntdev_priv *priv = file->private_data;
>  
> pr_debug("gntdev_vma_close %p\n", vma);
> -   if (use_ptemod && map->vma == vma) {
> +   if (use_ptemod) {
> +   WARN_ON(map->vma != vma);
> mmu_range_notifier_remove(>notifier);
> map->vma = NULL;
> }
>
> Jason

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

[Boris Ostrovsky]

On 11/4/19 9:31 PM, Jason Gunthorpe wrote:
> On Mon, Nov 04, 2019 at 05:03:31PM -0500, Boris Ostrovsky wrote:
>> On 10/28/19 4:10 PM, Jason Gunthorpe wrote:
>>> @@ -445,17 +438,9 @@ static void gntdev_vma_close(struct vm_area_struct 
>>> *vma)
>>> struct gntdev_priv *priv = file->private_data;
>>>  
>>> pr_debug("gntdev_vma_close %p\n", vma);
>>> -   if (use_ptemod) {
>>> -   /* It is possible that an mmu notifier could be running
>>> -* concurrently, so take priv->lock to ensure that the vma won't
>>> -* vanishing during the unmap_grant_pages call, since we will
>>> -* spin here until that completes. Such a concurrent call will
>>> -* not do any unmapping, since that has been done prior to
>>> -* closing the vma, but it may still iterate the unmap_ops list.
>>> -*/
>>> -   mutex_lock(>lock);
>>> +   if (use_ptemod && map->vma == vma) {
>>
>> Is it possible for map->vma not to be equal to vma?
> It could be NULL at least if use_ptemod is not set.
>
> Otherwise, I'm not sure, the confusing bit is that the map comes from
> here:
>
> map = gntdev_find_map_index(priv, index, count);
>
> It looks like the intent is that the map->vma is always set to the
> only vma that has the map as private_data.

I am not sure how this can work otherwise. We stash map pointer in vm's
vm_private_data and vice versa (for use_ptemod) gntdev_mmap() so if they
have to match.

That's why I was asking you to see if you had something particular in
mind when you added this test.

> So, I suppose it can be relaxed to a null test and a WARN_ON that it
> hasn't changed?

You mean

if (use_ptemod) {
    WARN_ON(map->vma != vma);
    ...


Yes, that sounds good.


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

[Boris Ostrovsky]

On 10/28/19 4:10 PM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe 
>
> gntdev simply wants to monitor a specific VMA for any notifier events,
> this can be done straightforwardly using mmu_range_notifier_insert() over
> the VMA's VA range.
>
> The notifier should be attached until the original VMA is destroyed.
>
> It is unclear if any of this is even sane, but at least a lot of duplicate
> code is removed.

I didn't have a chance to look at the patch itself yet but as a heads-up
--- it crashes dom0.

-boris


>
> Cc: Oleksandr Andrushchenko 
> Cc: Boris Ostrovsky 
> Cc: xen-de...@lists.xenproject.org
> Cc: Juergen Gross 
> Cc: Stefano Stabellini 
> Signed-off-by: Jason Gunthorpe 
> ---
>  drivers/xen/gntdev-common.h |   8 +-
>  drivers/xen/gntdev.c| 180 ++--
>  2 files changed, 49 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
> index 2f8b949c3eeb14..b201fdd20b667b 100644
> --- a/drivers/xen/gntdev-common.h
> +++ b/drivers/xen/gntdev-common.h
> @@ -21,15 +21,8 @@ struct gntdev_dmabuf_priv;
>  struct gntdev_priv {
>   /* Maps with visible offsets in the file descriptor. */
>   struct list_head maps;
> - /*
> -  * Maps that are not visible; will be freed on munmap.
> -  * Only populated if populate_freeable_maps == 1
> -  */
> - struct list_head freeable_maps;
>   /* lock protects maps and freeable_maps. */
>   struct mutex lock;
> - struct mm_struct *mm;
> - struct mmu_notifier mn;
>  
>  #ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>   /* Device for which DMA memory is allocated. */
> @@ -49,6 +42,7 @@ struct gntdev_unmap_notify {
>  };
>  
>  struct gntdev_grant_map {
> + struct mmu_range_notifier notifier;
>   struct list_head next;
>   struct vm_area_struct *vma;
>   int index;
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index a446a7221e13e9..12d626670bebbc 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -65,7 +65,6 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may 
> be mapped by "
>  static atomic_t pages_mapped = ATOMIC_INIT(0);
>  
>  static int use_ptemod;
> -#define populate_freeable_maps use_ptemod
>  
>  static int unmap_grant_pages(struct gntdev_grant_map *map,
>int offset, int pages);
> @@ -251,12 +250,6 @@ void gntdev_put_map(struct gntdev_priv *priv, struct 
> gntdev_grant_map *map)
>   evtchn_put(map->notify.event);
>   }
>  
> - if (populate_freeable_maps && priv) {
> - mutex_lock(>lock);
> - list_del(>next);
> - mutex_unlock(>lock);
> - }
> -
>   if (map->pages && !use_ptemod)
>   unmap_grant_pages(map, 0, map->count);
>   gntdev_free_map(map);
> @@ -445,17 +438,9 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
>   struct gntdev_priv *priv = file->private_data;
>  
>   pr_debug("gntdev_vma_close %p\n", vma);
> - if (use_ptemod) {
> - /* It is possible that an mmu notifier could be running
> -  * concurrently, so take priv->lock to ensure that the vma won't
> -  * vanishing during the unmap_grant_pages call, since we will
> -  * spin here until that completes. Such a concurrent call will
> -  * not do any unmapping, since that has been done prior to
> -  * closing the vma, but it may still iterate the unmap_ops list.
> -  */
> - mutex_lock(>lock);
> + if (use_ptemod && map->vma == vma) {
> + mmu_range_notifier_remove(>notifier);
>   map->vma = NULL;
> - mutex_unlock(>lock);
>   }
>   vma->vm_private_data = NULL;
>   gntdev_put_map(priv, map);
> @@ -477,109 +462,44 @@ static const struct vm_operations_struct gntdev_vmops 
> = {
>  
>  /* -- */
>  
> -static bool in_range(struct gntdev_grant_map *map,
> -   unsigned long start, unsigned long end)
> -{
> - if (!map->vma)
> - return false;
> - if (map->vma->vm_start >= end)
> - return false;
> - if (map->vma->vm_end <= start)
> - return false;
> -
> - return true;
> -}
> -
> -static int unmap_if_in_range(struct gntdev_grant_map *map,
> -   unsigned long start, unsigned long end,
> -   bool blockable)
> +sta

Re: [PATCH] dma-buf: add struct dma_buf_attach_info v2

[Boris Ostrovsky]

On 4/30/19 7:10 AM, Christian König wrote:
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 2c4f324f8626..cacca830b482 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -608,6 +608,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, 
> struct device *dev,
>  int fd, int count, int domid)
>  {
>   struct gntdev_dmabuf *gntdev_dmabuf, *ret;
> + struct dma_buf_attach_info attach_info;
>   struct dma_buf *dma_buf;
>   struct dma_buf_attachment *attach;
>   struct sg_table *sgt;
> @@ -627,6 +628,9 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, 
> struct device *dev,
>   gntdev_dmabuf->priv = priv;
>   gntdev_dmabuf->fd = fd;
>  
> + memset(_info, 0, sizeof(attach_info));
> + attach_info.dev = dev;
> + attach_info.dmabuf = dma_buf;
>   attach = dma_buf_attach(dma_buf, dev);
>   if (IS_ERR(attach)) {
>   ret = ERR_CAST(attach);

This won't build.

Did you mean

    attach = dma_buf_attach(_info);

?

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel][PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

[Boris Ostrovsky]

On 12/18/18 11:15 AM, Noralf Trønnes wrote:
>
> Den 30.11.2018 08.42, skrev Oleksandr Andrushchenko:
>> From: Oleksandr Andrushchenko 
>>
>> Use page directory based shared buffer implementation
>> now available as common code for Xen frontend drivers.
>>
>> Remove flushing of shared buffer on page flip as this
>> workaround needs a proper fix.
>>
>> Signed-off-by: Oleksandr Andrushchenko
>> 
>> ---
>
> Reviewed-by: Noralf Trønnes 
>


Now that all 3 have been acked/reviewed

Applied to for-linus-4.21


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

[Boris Ostrovsky]

On 12/17/18 5:19 AM, Oleksandr Andrushchenko wrote:
> Hello, Juergen, Boris!
>
> As this DRM part of the series is the only one which needs ack/nack
>
> (and it might take quite some time to complete) could we please
>
> merge the patches 1 and 3 now that already have ack/r-b?
>



TBH I am not sure it makes sense to do this without the second patch.
Refactoring (and IIUIC this series is purely refactoring --- is it not?)
is done to reduce amount of code, and with only first and third patch we
end up with quite a significant increase in the number of LoC. (I am
going purely by diffstat)

Of course, the other reason for refactoring is to eliminate code
duplication, but without second patch that will not happen.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v2 2/3] drm/xen-front: Use Xen common shared buffer implementation

[Boris Ostrovsky]

On 12/17/18 10:03 AM, Oleksandr Andrushchenko wrote:
> On 12/17/18 4:52 PM, Boris Ostrovsky wrote:
>> On 12/17/18 5:19 AM, Oleksandr Andrushchenko wrote:
>>> Hello, Juergen, Boris!
>>>
>>> As this DRM part of the series is the only one which needs ack/nack
>>>
>>> (and it might take quite some time to complete) could we please
>>>
>>> merge the patches 1 and 3 now that already have ack/r-b?
>>>
>>
>>
>> TBH I am not sure it makes sense to do this without the second patch.
>> Refactoring (and IIUIC this series is purely refactoring --- is it not?)
>> is done to reduce amount of code, and with only first and third patch we
>> end up with quite a significant increase in the number of LoC. (I am
>> going purely by diffstat)
>>
>> Of course, the other reason for refactoring is to eliminate code
>> duplication, but without second patch that will not happen.
>
> Agree, but this is the basis for the new pv camera frontend
>
> I am working on now [1], so even if we do not remove the code from DRM
>
> then we at least do not add it to the camera driver


Since 1 and 3 are already ACKed you should be able to start the camera
series with these two patches as pre-requisites even if patch 2 is still
stalled by the time your camera code is posted (which I assume will be
4.22 or later).



-boris


>
>> -boris
>
> Thank you,
>
> Oleksandr
>
> [1]
> https://github.com/andr2000/linux/blob/camera_front_v1/drivers/media/xen/Kconfig#L6
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel][PATCH v2 1/3] xen: Introduce shared buffer helpers for page directory...

[Boris Ostrovsky]

On 11/30/18 2:42 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> based frontends. Currently the frontends which implement
> similar code for sharing big buffers between frontend and
> backend are para-virtualized DRM and sound drivers.
> Both define the same way to share grant references of a
> data buffer with the corresponding backend with little
> differences.
>
> Move shared code into a helper module, so there is a single
> implementation of the same functionality for all.
>
> This patch introduces code which is used by sound and display
> frontend drivers without functional changes with the intention
> to remove shared code from the corresponding drivers.
>
> Signed-off-by: Oleksandr Andrushchenko 

Acked-by: Boris Ostrovsky 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/9] Use vm_insert_range

[Boris Ostrovsky]

On 11/21/18 1:24 AM, Souptick Joarder wrote:
> On Thu, Nov 15, 2018 at 9:09 PM Souptick Joarder  wrote:
>> Previouly drivers have their own way of mapping range of
>> kernel pages/memory into user vma and this was done by
>> invoking vm_insert_page() within a loop.
>>
>> As this pattern is common across different drivers, it can
>> be generalized by creating a new function and use it across
>> the drivers.
>>
>> vm_insert_range is the new API which will be used to map a
>> range of kernel memory/pages to user vma.
>>
>> All the applicable places are converted to use new vm_insert_range
>> in this patch series.
>>
>> Souptick Joarder (9):
>>   mm: Introduce new vm_insert_range API
>>   arch/arm/mm/dma-mapping.c: Convert to use vm_insert_range
>>   drivers/firewire/core-iso.c: Convert to use vm_insert_range
>>   drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range
>>   drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range
>>   iommu/dma-iommu.c: Convert to use vm_insert_range
>>   videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range
>>   xen/gntdev.c: Convert to use vm_insert_range
>>   xen/privcmd-buf.c: Convert to use vm_insert_range
> Any further comment on driver changes ?

Xen drivers (the last two patches) look fine to me.

-boris


>>  arch/arm/mm/dma-mapping.c | 21 ++---
>>  drivers/firewire/core-iso.c   | 15 ++--
>>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c   | 20 ++--
>>  drivers/gpu/drm/xen/xen_drm_front_gem.c   | 20 +---
>>  drivers/iommu/dma-iommu.c | 12 ++
>>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 23 ++-
>>  drivers/xen/gntdev.c  | 11 -
>>  drivers/xen/privcmd-buf.c |  8 ++-
>>  include/linux/mm_types.h  |  3 +++
>>  mm/memory.c   | 28 
>> +++
>>  mm/nommu.c|  7 ++
>>  11 files changed, 70 insertions(+), 98 deletions(-)
>>
>> --
>> 1.9.1
>>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/9] Use vm_insert_range

[Boris Ostrovsky]

On 11/21/18 2:56 PM, Souptick Joarder wrote:
> On Thu, Nov 22, 2018 at 1:08 AM Boris Ostrovsky
>  wrote:
>> On 11/21/18 1:24 AM, Souptick Joarder wrote:
>>> On Thu, Nov 15, 2018 at 9:09 PM Souptick Joarder  
>>> wrote:
>>>> Previouly drivers have their own way of mapping range of
>>>> kernel pages/memory into user vma and this was done by
>>>> invoking vm_insert_page() within a loop.
>>>>
>>>> As this pattern is common across different drivers, it can
>>>> be generalized by creating a new function and use it across
>>>> the drivers.
>>>>
>>>> vm_insert_range is the new API which will be used to map a
>>>> range of kernel memory/pages to user vma.
>>>>
>>>> All the applicable places are converted to use new vm_insert_range
>>>> in this patch series.
>>>>
>>>> Souptick Joarder (9):
>>>>   mm: Introduce new vm_insert_range API
>>>>   arch/arm/mm/dma-mapping.c: Convert to use vm_insert_range
>>>>   drivers/firewire/core-iso.c: Convert to use vm_insert_range
>>>>   drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range
>>>>   drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range
>>>>   iommu/dma-iommu.c: Convert to use vm_insert_range
>>>>   videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range
>>>>   xen/gntdev.c: Convert to use vm_insert_range
>>>>   xen/privcmd-buf.c: Convert to use vm_insert_range
>>> Any further comment on driver changes ?
>> Xen drivers (the last two patches) look fine to me.
> Thanks, can I considered this as Reviewed-by ?


Reviewed-by: Boris Ostrovsky 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 0/8] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 07/20/2018 05:01 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> This work is in response to my previous attempt to introduce Xen/DRM
> zero-copy driver [1] to enable Linux dma-buf API [2] for Xen based
> frontends/backends. There is also an existing hyper_dmabuf approach
> available [3] which, if reworked to utilize the proposed solution,
> can greatly benefit as well.
>


Applied to for-linus-4.19.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 0/8] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 07/23/2018 09:26 AM, Oleksandr Andrushchenko wrote:
> On 07/23/2018 11:38 AM, Oleksandr Andrushchenko wrote:
>>
>>> data/upstream/linux-xen/drivers/xen/gntdev-dmabuf.c: In function
>>> ‘gntdev_ioctl_dmabuf_exp_from_refs’:
>>> /data/upstream/linux-xen/drivers/xen/gntdev-dmabuf.c:503:6: warning:
>>> ‘args.fd’ may be used uninitialized in this function
>>> [-Wmaybe-uninitialized]
>>>    *fd = args.fd;
>>>    ^
>>> /data/upstream/linux-xen/drivers/xen/gntdev-dmabuf.c:467:35: note:
>>> ‘args.fd’ was declared here
>>>    struct gntdev_dmabuf_export_args args;
>>>     ^~~~
>> Strangely, but my i386 build goes smooth.
>> Which version of gcc you use and could you please give me your
>> .config, so I can test the same?
> Now I see this warning which seems to be a false positive.
> Boris, could you please apply the following:
>
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index e4c9f1f74476..0680dbcba616 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -495,6 +495,7 @@ static int dmabuf_exp_from_refs(struct gntdev_priv
> *priv, int flags,
>     args.dmabuf_priv = priv->dmabuf_priv;
>     args.count = map->count;
>     args.pages = map->pages;
> +   args.fd = -1;
>
>     ret = dmabuf_exp_from_pages();
>     if (ret < 0)
>
> or please let me know if you want me to resend with this fix?


Missed this message. Yes, this obviously fixes the problem. And it is
due to the code fragment that I mentioned in the earlier response.

Which patch is this for? I can add this when committing.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 0/8] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 07/20/2018 05:01 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> This work is in response to my previous attempt to introduce Xen/DRM
> zero-copy driver [1] to enable Linux dma-buf API [2] for Xen based
> frontends/backends. There is also an existing hyper_dmabuf approach
> available [3] which, if reworked to utilize the proposed solution,
> can greatly benefit as well.


Lot of warnings on  i386 build:

In file included from
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.c:24:
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.h: In
function ‘xen_drm_front_fb_to_cookie’:
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.h:129:9:
warning: cast from pointer to integer of different size
[-Wpointer-to-int-cast]
  return (u64)fb;
 ^
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.h: In
function ‘xen_drm_front_dbuf_to_cookie’:
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.h:134:9:
warning: cast from pointer to integer of different size
[-Wpointer-to-int-cast]
  return (u64)gem_obj;
 ^
  CC [M]  net/netfilter/ipset/ip_set_hash_ipport.o
  CC  drivers/media/rc/keymaps/rc-tango.o
  CC [M]  drivers/gpu/drm/vmwgfx/vmwgfx_fifo.o
  AR  drivers/misc/built-in.a
In file included from
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front_kms.c:20:
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.h: In
function ‘xen_drm_front_fb_to_cookie’:
  CC [M]  drivers/gpu/drm/xen/xen_drm_front_conn.o
/data/upstream/linux-xen/drivers/gpu/drm/xen/xen_drm_front.h:129:9:
warning: cast from pointer to integer of different size
[-Wpointer-to-int-cast]
  return (u64)fb;
(and more)



and then

data/upstream/linux-xen/drivers/xen/gntdev-dmabuf.c: In function
‘gntdev_ioctl_dmabuf_exp_from_refs’:
/data/upstream/linux-xen/drivers/xen/gntdev-dmabuf.c:503:6: warning:
‘args.fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  *fd = args.fd;
  ^
/data/upstream/linux-xen/drivers/xen/gntdev-dmabuf.c:467:35: note:
‘args.fd’ was declared here
  struct gntdev_dmabuf_export_args args;
   ^~~~


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 0/9] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 07/02/2018 09:12 AM, Oleksandr Andrushchenko wrote:
> On 07/02/2018 11:20 AM, Juergen Gross wrote:
>> On 02/07/18 09:10, Oleksandr Andrushchenko wrote:
>>> Hello, Boris, Juergen!
>>>
>>> Do you think I can re-base the series (which already has
>>> all required R-b's from Xen community) onto the latest kernel
>>> with API changes to patches 5 (of_dma_configure) and 8
>>> (dma-buf atomic ops) and we can merge it to the Xen's kernel tree?
>> Rebase: yes.
>>
>> Merging to the Xen kernel tree: only after setting up the
>> for-linus-4.19 branch, which will be done by Boris later this
>> month.
> Then I'll probably have to wait until for-linus-4.19 branch
> Boris, do you have any dates in mind for that?


Usually after rc5, so I suspect sometime next week.


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/9] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 06/14/2018 02:47 AM, Oleksandr Andrushchenko wrote:
> Hi, Boris!
>
> It seems that I have resolved all the issues now which
> were mainly cleanup and code movement and 5 of 9 patches
> already have r-b's. Do you, as the primary reviewer of the
> series, think I can push (hopefully) the final version
> of the patches? Just in case you want to look at v4 it is at [1].
>
> Thank you,
> Oleksandr
>
> [1]
> https://github.com/andr2000/linux/commits/xen_tip_linux_next_xen_dma_buf_v4


Looks good to me.


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 9/9] xen/gntdev: Implement dma-buf import functionality

[Boris Ostrovsky]

On 06/13/2018 05:04 AM, Oleksandr Andrushchenko wrote:
> On 06/13/2018 06:14 AM, Boris Ostrovsky wrote:
>>
>>
>> On 06/12/2018 09:42 AM, Oleksandr Andrushchenko wrote:
>>
>>>   int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32
>>> fd)
>>>   {
>>> -    return -EINVAL;
>>> +    struct gntdev_dmabuf *gntdev_dmabuf;
>>> +    struct dma_buf_attachment *attach;
>>> +    struct dma_buf *dma_buf;
>>> +
>>> +    gntdev_dmabuf = dmabuf_imp_find_unlink(priv, fd);
>>> +    if (IS_ERR(gntdev_dmabuf))
>>> +    return PTR_ERR(gntdev_dmabuf);
>>> +
>>> +    pr_debug("Releasing DMA buffer with fd %d\n", fd);
>>> +
>>> +    attach = gntdev_dmabuf->u.imp.attach;
>>> +
>>> +    if (gntdev_dmabuf->u.imp.sgt)
>>> +    dma_buf_unmap_attachment(attach, gntdev_dmabuf->u.imp.sgt,
>>> + DMA_BIDIRECTIONAL);
>>> +    dma_buf = attach->dmabuf;
>>> +    dma_buf_detach(attach->dmabuf, attach);
>>> +    dma_buf_put(dma_buf);
>>> +
>>> +    dmabuf_imp_end_foreign_access(gntdev_dmabuf->u.imp.refs,
>>> +  gntdev_dmabuf->nr_pages);
>>
>>
>>
>> Should you first end foreign access, before doing anything?
>>
> I am rolling back in reverse order here, so I think we first need
> to finish local activities with the buffer and then end foreign
> access.

Looking at gntdev_dmabuf_imp_to_refs(), the order is
    dmabuf_imp_alloc_storage()
    dma_buf_attach()
    dma_buf_map_attachment()
    dmabuf_imp_grant_foreign_access()

Or was I looking at wrong place?

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

On 06/13/2018 07:57 AM, Oleksandr Andrushchenko wrote:
> On 06/13/2018 05:58 AM, Boris Ostrovsky wrote:
>>
>>
>> On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:
>>>
>>> +
>>> +static struct gntdev_dmabuf *
>>> +dmabuf_exp_wait_obj_get_by_fd(struct gntdev_dmabuf_priv *priv, int fd)
>>
>>
>> The name of this routine implies (to me) that we are getting a wait
>> object but IIUIC we are getting a gntdev_dmabuf that we are going to
>> later associate with a wait object.
>>
> How about dmabuf_exp_wait_obj_get_dmabuf_by_fd?
> I would like to keep function prefixes, e.g. dmabuf_exp_wait_obj_
> just to show to which functionality a routine belongs.


That's too long IMO. If you really want to keep the prefix then let's
keep this the way it is. Maybe it's just me who read it that way.


>>
>>>
>>>   +#ifdef CONFIG_XEN_GNTDEV_DMABUF
>>> +void gntdev_remove_map(struct gntdev_priv *priv, struct
>>> gntdev_grant_map *map)
>>> +{
>>> +    mutex_lock(>lock);
>>> +    list_del(>next);
>>> +    gntdev_put_map(NULL /* already removed */, map);
>>
>>
>> Why not pass call gntdev_put_map(priv, map) and then not have this
>> routine at all?
>>
> Well, I wish I could, but the main difference when calling
> gntdev_put_map(priv, map)
> with priv != NULL and my code is that:
>
> void gntdev_put_map(struct gntdev_priv *priv, struct gntdev_grant_map
> *map)
> {
>     [...]
>     if (populate_freeable_maps && priv) {
>     ^
>         mutex_lock(>lock);
>         list_del(>next);
>         mutex_unlock(>lock);
>     }
>     [...]
> }
>
> and
>
> #define populate_freeable_maps use_ptemod
>                            ^^
> which means the map will never be removed from the list in my case
> because use_ptemod == false for dma-buf.
> This is why I do that by hand, e.g. remove the item from the list
> and then pass NULL for priv.
>
> Also, I will remove gntdev_remove_map as I can now access
> priv->lock and gntdev_put_map directly form gntdev-dmabuf.c


Yes, that's a good idea.

>
>> I really dislike the fact that we are taking a lock here that
>> gntdev_put_map() takes as well, although not with NULL argument. (And
>> yes, I see that gntdev_release() does it too.)
>>
> This can be re-factored later I guess?

OK.

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/9] xen/balloon: Share common memory reservation routines

[Boris Ostrovsky]

On 06/13/2018 02:26 AM, Oleksandr Andrushchenko wrote:

On 06/13/2018 03:47 AM, Boris Ostrovsky wrote:



On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 


diff --git a/include/xen/mem-reservation.h 
b/include/xen/mem-reservation.h

new file mode 100644
index ..e0939387278d
--- /dev/null
+++ b/include/xen/mem-reservation.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Xen memory reservation utilities.
+ *
+ * Copyright (c) 2003, B Dragovic
+ * Copyright (c) 2003-2004, M Williamson, K Fraser
+ * Copyright (c) 2005 Dan M. Smith, IBM Corporation
+ * Copyright (c) 2010 Daniel Kiper
+ * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
+ */
+
+#ifndef _XENMEM_RESERVATION_H
+#define _XENMEM_RESERVATION_H
+
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+#include 



I should have noticed this in the previous post but I suspect most of 
these includes belong in the C file. For example, there is no reason 
for hypercall.h here.



Yes, it seems that the header can only have
#include 
Will move the rest into the .c file



You may need something for clear_highpage() and maybe for Xen feature 
flags. But you'll find out for sure when you try to build. ;-)


-boris




-boris



+
+static inline void xenmem_reservation_scrub_page(struct page *page)
+{
+#ifdef CONFIG_XEN_SCRUB_PAGES
+    clear_highpage(page);
+#endif
+}
+
+#ifdef CONFIG_XEN_HAVE_PVMMU
+void __xenmem_reservation_va_mapping_update(unsigned long count,
+    struct page **pages,
+    xen_pfn_t *frames);
+
+void __xenmem_reservation_va_mapping_reset(unsigned long count,
+   struct page **pages);
+#endif
+
+static inline void xenmem_reservation_va_mapping_update(unsigned 
long count,

+    struct page **pages,
+    xen_pfn_t *frames)
+{
+#ifdef CONFIG_XEN_HAVE_PVMMU
+    if (!xen_feature(XENFEAT_auto_translated_physmap))
+    __xenmem_reservation_va_mapping_update(count, pages, frames);
+#endif
+}
+
+static inline void xenmem_reservation_va_mapping_reset(unsigned long 
count,

+   struct page **pages)
+{
+#ifdef CONFIG_XEN_HAVE_PVMMU
+    if (!xen_feature(XENFEAT_auto_translated_physmap))
+    __xenmem_reservation_va_mapping_reset(count, pages);
+#endif
+}
+
+int xenmem_reservation_increase(int count, xen_pfn_t *frames);
+
+int xenmem_reservation_decrease(int count, xen_pfn_t *frames);
+
+#endif




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/9] xen/balloon: Share common memory reservation routines

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 



diff --git a/include/xen/mem-reservation.h b/include/xen/mem-reservation.h
new file mode 100644
index ..e0939387278d
--- /dev/null
+++ b/include/xen/mem-reservation.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Xen memory reservation utilities.
+ *
+ * Copyright (c) 2003, B Dragovic
+ * Copyright (c) 2003-2004, M Williamson, K Fraser
+ * Copyright (c) 2005 Dan M. Smith, IBM Corporation
+ * Copyright (c) 2010 Daniel Kiper
+ * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
+ */
+
+#ifndef _XENMEM_RESERVATION_H
+#define _XENMEM_RESERVATION_H
+
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+#include 



I should have noticed this in the previous post but I suspect most of 
these includes belong in the C file. For example, there is no reason for 
hypercall.h here.


-boris



+
+static inline void xenmem_reservation_scrub_page(struct page *page)
+{
+#ifdef CONFIG_XEN_SCRUB_PAGES
+   clear_highpage(page);
+#endif
+}
+
+#ifdef CONFIG_XEN_HAVE_PVMMU
+void __xenmem_reservation_va_mapping_update(unsigned long count,
+   struct page **pages,
+   xen_pfn_t *frames);
+
+void __xenmem_reservation_va_mapping_reset(unsigned long count,
+  struct page **pages);
+#endif
+
+static inline void xenmem_reservation_va_mapping_update(unsigned long count,
+   struct page **pages,
+   xen_pfn_t *frames)
+{
+#ifdef CONFIG_XEN_HAVE_PVMMU
+   if (!xen_feature(XENFEAT_auto_translated_physmap))
+   __xenmem_reservation_va_mapping_update(count, pages, frames);
+#endif
+}
+
+static inline void xenmem_reservation_va_mapping_reset(unsigned long count,
+  struct page **pages)
+{
+#ifdef CONFIG_XEN_HAVE_PVMMU
+   if (!xen_feature(XENFEAT_auto_translated_physmap))
+   __xenmem_reservation_va_mapping_reset(count, pages);
+#endif
+}
+
+int xenmem_reservation_increase(int count, xen_pfn_t *frames);
+
+int xenmem_reservation_decrease(int count, xen_pfn_t *frames);
+
+#endif


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/9] xen/grant-table: Allow allocating buffers suitable for DMA

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Extend grant table module API to allow allocating buffers that can
be used for DMA operations and mapping foreign grant references
on top of those.
The resulting buffer is similar to the one allocated by the balloon
driver in terms that proper memory reservation is made
({increase|decrease}_reservation and VA mappings updated if needed).
This is useful for sharing foreign buffers with HW drivers which
cannot work with scattered buffers provided by the balloon driver,
but require DMAable memory instead.

Signed-off-by: Oleksandr Andrushchenko 


Reviewed-by: Boris Ostrovsky 

with a small nit below



---
  drivers/xen/Kconfig   | 13 ++
  drivers/xen/grant-table.c | 97 +++
  include/xen/grant_table.h | 18 
  3 files changed, 128 insertions(+)

diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index e5d0c28372ea..39536ddfbce4 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -161,6 +161,19 @@ config XEN_GRANT_DEV_ALLOC
  to other domains. This can be used to implement frontend drivers
  or as part of an inter-domain shared memory channel.
  
+config XEN_GRANT_DMA_ALLOC

+   bool "Allow allocating DMA capable buffers with grant reference module"
+   depends on XEN && HAS_DMA
+   help
+ Extends grant table module API to allow allocating DMA capable
+ buffers and mapping foreign grant references on top of it.
+ The resulting buffer is similar to one allocated by the balloon
+ driver in terms that proper memory reservation is made
+ ({increase|decrease}_reservation and VA mappings updated if needed).


I think you should drop the word "terms" and say "is made *by*" and "VA 
mappings *are* updated"


And similar change in the commit message.

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 5/9] xen/gntdev: Allow mappings for DMA buffers

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:

  
  static void gntdev_print_maps(struct gntdev_priv *priv,

@@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map)
if (map == NULL)
return;
  
+#ifdef CONFIG_XEN_GRANT_DMA_ALLOC

+   if (map->dma_vaddr) {
+   struct gnttab_dma_alloc_args args;
+
+   args.dev = map->dma_dev;
+   args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT;



args.coherent = !!(map->dma_flags & GNTDEV_DMA_FLAG_COHERENT);



+   args.nr_pages = map->count;
+   args.pages = map->pages;
+   args.frames = map->frames;
+   args.vaddr = map->dma_vaddr;
+   args.dev_bus_addr = map->dma_bus_addr;
+
+   gnttab_dma_free_pages();
+   } else
+#endif
if (map->pages)
gnttab_free_pages(map->count, map->pages);
+
+#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
+   kfree(map->frames);
+#endif
kfree(map->pages);
kfree(map->grants);
kfree(map->map_ops);
@@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map)
kfree(map);
  }
  
-static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)

+static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count,
+ int dma_flags)
  {
struct grant_map *add;
int i;
@@ -155,6 +200,37 @@ static struct grant_map *gntdev_alloc_map(struct 
gntdev_priv *priv, int count)
NULL == add->pages)
goto err;
  
+#ifdef CONFIG_XEN_GRANT_DMA_ALLOC

+   add->dma_flags = dma_flags;
+
+   /*
+* Check if this mapping is requested to be backed
+* by a DMA buffer.
+*/
+   if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) {
+   struct gnttab_dma_alloc_args args;
+
+   add->frames = kcalloc(count, sizeof(add->frames[0]),
+ GFP_KERNEL);
+   if (!add->frames)
+   goto err;
+
+   /* Remember the device, so we can free DMA memory. */
+   add->dma_dev = priv->dma_dev;
+
+   args.dev = priv->dma_dev;
+   args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT;



And again here.



+   args.nr_pages = count;
+   args.pages = add->pages;
+   args.frames = add->frames;
+
+   if (gnttab_dma_alloc_pages())
+   goto err;
+
+   add->dma_vaddr = args.vaddr;
+   add->dma_bus_addr = args.dev_bus_addr;
+   } else
+#endif
if (gnttab_alloc_pages(count, add->pages))
goto err;
  
@@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map *map)

map->unmap_ops[i].handle = map->map_ops[i].handle;
if (use_ptemod)
map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
+#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
+   else if (map->dma_vaddr) {
+   unsigned long mfn;



This should be called bfn now.



+
+   mfn = pfn_to_bfn(page_to_pfn(map->pages[i]));
+   map->unmap_ops[i].dev_bus_addr = __pfn_to_phys(mfn);
+   }
+#endif
}
return err;
  }
@@ -548,6 +632,17 @@ static int gntdev_open(struct inode *inode, struct file 
*flip)
}
  
  	flip->private_data = priv;

+#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
+   priv->dma_dev = gntdev_miscdev.this_device;
+
+   /*
+* The device is not spawn from a device tree, so arch_setup_dma_ops
+* is not called, thus leaving the device with dummy DMA ops.
+* Fix this call of_dma_configure() with a NULL node to set



"Fix this by calling ..." I think.



+* default DMA ops.
+*/
+   of_dma_configure(priv->dma_dev, NULL);
+#endif
pr_debug("priv %p\n", priv);
  
  	return 0;



With those fixed,

Reviewed-by: Boris Ostrovsky 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/9] xen/gntdev: Make private routines/structures accessible

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

This is in preparation for adding support of DMA buffer
functionality: make map/unmap related code and structures, used
privately by gntdev, ready for dma-buf extension, which will re-use
these. Rename corresponding structures as those become non-private
to gntdev now.

Signed-off-by: Oleksandr Andrushchenko 
---
  drivers/xen/gntdev-common.h |  86 +++
  drivers/xen/gntdev.c| 132 
  2 files changed, 128 insertions(+), 90 deletions(-)
  create mode 100644 drivers/xen/gntdev-common.h

diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
new file mode 100644
index ..7a9845a6bee9
--- /dev/null
+++ b/drivers/xen/gntdev-common.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Common functionality of grant device.
+ *
+ * Copyright (c) 2006-2007, D G Murray.
+ *   (c) 2009 Gerd Hoffmann 
+ *   (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
+ */
+
+#ifndef _GNTDEV_COMMON_H
+#define _GNTDEV_COMMON_H
+
+#include 
+#include 
+#include 
+#include 
+
+struct gntdev_priv {
+   /* maps with visible offsets in the file descriptor */
+   struct list_head maps;
+   /* maps that are not visible; will be freed on munmap.
+* Only populated if populate_freeable_maps == 1 */



Since you are touching this code please fix comment style.



+   struct list_head freeable_maps;
+   /* lock protects maps and freeable_maps */
+   struct mutex lock;
+   struct mm_struct *mm;
+   struct mmu_notifier mn;
+
+#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
+   /* Device for which DMA memory is allocated. */
+   struct device *dma_dev;
+#endif
+};



With that fixed,

Reviewed-by: Boris Ostrovsky 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/9] xen/balloon: Share common memory reservation routines

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:



One more thing: please add a comment here saying that frames array is 
array of PFNs (in Xen granularity), which is what 
XENMEM_populate_physmap requires. And remove (or update to name the 
actual call you are making) the corresponding comment in 
increase_reservation().




+
+int xenmem_reservation_increase(int count, xen_pfn_t *frames)
+{
+   struct xen_memory_reservation reservation = {
+   .address_bits = 0,
+   .extent_order = EXTENT_ORDER,
+   .domid= DOMID_SELF
+   };
+
+   set_xen_guest_handle(reservation.extent_start, frames);
+   reservation.nr_extents = count;
+   return HYPERVISOR_memory_op(XENMEM_populate_physmap, );
+}
+EXPORT_SYMBOL_GPL(xenmem_reservation_increase);



And similarly, here we are requesting GFNs, and update 
decrease_reservation().



-boris


+
+int xenmem_reservation_decrease(int count, xen_pfn_t *frames)
+{
+   struct xen_memory_reservation reservation = {
+   .address_bits = 0,
+   .extent_order = EXTENT_ORDER,
+   .domid= DOMID_SELF
+   };
+
+   set_xen_guest_handle(reservation.extent_start, frames);
+   reservation.nr_extents = count;
+   return HYPERVISOR_memory_op(XENMEM_decrease_reservation, );

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 9/9] xen/gntdev: Implement dma-buf import functionality

[Boris Ostrovsky]

On 06/12/2018 09:42 AM, Oleksandr Andrushchenko wrote:


  int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd)
  {
-   return -EINVAL;
+   struct gntdev_dmabuf *gntdev_dmabuf;
+   struct dma_buf_attachment *attach;
+   struct dma_buf *dma_buf;
+
+   gntdev_dmabuf = dmabuf_imp_find_unlink(priv, fd);
+   if (IS_ERR(gntdev_dmabuf))
+   return PTR_ERR(gntdev_dmabuf);
+
+   pr_debug("Releasing DMA buffer with fd %d\n", fd);
+
+   attach = gntdev_dmabuf->u.imp.attach;
+
+   if (gntdev_dmabuf->u.imp.sgt)
+   dma_buf_unmap_attachment(attach, gntdev_dmabuf->u.imp.sgt,
+DMA_BIDIRECTIONAL);
+   dma_buf = attach->dmabuf;
+   dma_buf_detach(attach->dmabuf, attach);
+   dma_buf_put(dma_buf);
+
+   dmabuf_imp_end_foreign_access(gntdev_dmabuf->u.imp.refs,
+ gntdev_dmabuf->nr_pages);




Should you first end foreign access, before doing anything?

-boris



+   dmabuf_imp_free_storage(gntdev_dmabuf);
+   return 0;
  }
  

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 8/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

1. Create a dma-buf from grant references provided by the foreign
domain. By default dma-buf is backed by system memory pages, but
by providing GNTDEV_DMA_FLAG_XXX flags it can also be created
as a DMA write-combine/coherent buffer, e.g. allocated with
corresponding dma_alloc_xxx API.
Export the resulting buffer as a new dma-buf.

2. Implement waiting for the dma-buf to be released: block until the
dma-buf with the file descriptor provided is released.
If within the time-out provided the buffer is not released then
-ETIMEDOUT error is returned. If the buffer with the file descriptor
does not exist or has already been released, then -ENOENT is
returned. For valid file descriptors this must not be treated as
error.

3. Make gntdev's common code and structures available to dma-buf.

Signed-off-by: Oleksandr Andrushchenko 
---
  drivers/xen/gntdev-common.h |   4 +
  drivers/xen/gntdev-dmabuf.c | 470 +++-
  drivers/xen/gntdev.c|  10 +
  3 files changed, 482 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
index a3408fd39b07..72f80dbce861 100644
--- a/drivers/xen/gntdev-common.h
+++ b/drivers/xen/gntdev-common.h
@@ -89,4 +89,8 @@ bool gntdev_account_mapped_pages(int count);
  
  int gntdev_map_grant_pages(struct gntdev_grant_map *map);
  
+#ifdef CONFIG_XEN_GNTDEV_DMABUF

+void gntdev_remove_map(struct gntdev_priv *priv, struct gntdev_grant_map *map);
+#endif
+
  #endif
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index dc57c6a25525..84cba67c6ad7 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -3,13 +3,53 @@
  /*
   * Xen dma-buf functionality for gntdev.
   *
+ * DMA buffer implementation is based on drivers/gpu/drm/drm_prime.c.
+ *
   * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
   */
  
+#include 

  #include 
  
+#include 

+#include 
+
+#include "gntdev-common.h"
  #include "gntdev-dmabuf.h"
  
+struct gntdev_dmabuf {

+   struct gntdev_dmabuf_priv *priv;
+   struct dma_buf *dmabuf;
+   struct list_head next;
+   int fd;
+
+   union {
+   struct {
+   /* Exported buffers are reference counted. */
+   struct kref refcount;
+
+   struct gntdev_priv *priv;
+   struct gntdev_grant_map *map;
+   } exp;
+   } u;
+
+   /* Number of pages this buffer has. */
+   int nr_pages;
+   /* Pages of this buffer. */
+   struct page **pages;
+};
+
+struct gntdev_dmabuf_wait_obj {
+   struct list_head next;
+   struct gntdev_dmabuf *gntdev_dmabuf;
+   struct completion completion;
+};
+
+struct gntdev_dmabuf_attachment {
+   struct sg_table *sgt;
+   enum dma_data_direction dir;
+};
+
  struct gntdev_dmabuf_priv {
/* List of exported DMA buffers. */
struct list_head exp_list;
@@ -23,17 +63,439 @@ struct gntdev_dmabuf_priv {
  
  /* Implementation of wait for exported DMA buffer to be released. */
  
+static void dmabuf_exp_release(struct kref *kref);

+
+static struct gntdev_dmabuf_wait_obj *
+dmabuf_exp_wait_obj_new(struct gntdev_dmabuf_priv *priv,
+   struct gntdev_dmabuf *gntdev_dmabuf)
+{
+   struct gntdev_dmabuf_wait_obj *obj;
+
+   obj = kzalloc(sizeof(*obj), GFP_KERNEL);
+   if (!obj)
+   return ERR_PTR(-ENOMEM);
+
+   init_completion(>completion);
+   obj->gntdev_dmabuf = gntdev_dmabuf;
+
+   mutex_lock(>lock);
+   list_add(>next, >exp_wait_list);
+   /* Put our reference and wait for gntdev_dmabuf's release to fire. */
+   kref_put(_dmabuf->u.exp.refcount, dmabuf_exp_release);
+   mutex_unlock(>lock);
+   return obj;
+}
+
+static void dmabuf_exp_wait_obj_free(struct gntdev_dmabuf_priv *priv,
+struct gntdev_dmabuf_wait_obj *obj)
+{
+   struct gntdev_dmabuf_wait_obj *cur_obj, *q;
+
+   mutex_lock(>lock);
+   list_for_each_entry_safe(cur_obj, q, >exp_wait_list, next)
+   if (cur_obj == obj) {
+   list_del(>next);
+   kfree(obj);
+   break;
+   }
+   mutex_unlock(>lock);



Do we really need to walk the list?

And if we do, do we need the safe variant of the walk? We are holding 
the lock. Here and elsewhere.




+}
+
+static int dmabuf_exp_wait_obj_wait(struct gntdev_dmabuf_wait_obj *obj,
+   u32 wait_to_ms)
+{
+   if (wait_for_completion_timeout(>completion,
+   msecs_to_jiffies(wait_to_ms)) <= 0)
+   return -ETIMEDOUT;
+
+   return 0;
+}
+
+static void dmabuf_exp_wait_obj_signal(struct gntdev_dmabuf_priv *priv,
+  struct 

Re: [PATCH v3 7/9] xen/gntdev: Add initial support for dma-buf UAPI

[Boris Ostrovsky]

On 06/12/2018 09:41 AM, Oleksandr Andrushchenko wrote:


diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index a09db23e9663..e82660d81d7e 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -48,6 +48,9 @@
  #include 
  
  #include "gntdev-common.h"

+#ifdef CONFIG_XEN_GNTDEV_DMABUF
+#include "gntdev-dmabuf.h"
+#endif
  
  MODULE_LICENSE("GPL");

  MODULE_AUTHOR("Derek G. Murray , "
@@ -566,6 +569,15 @@ static int gntdev_open(struct inode *inode, struct file 
*flip)
INIT_LIST_HEAD(>freeable_maps);
mutex_init(>lock);
  
+#ifdef CONFIG_XEN_GNTDEV_DMABUF

+   priv->dmabuf_priv = gntdev_dmabuf_init();
+   if (IS_ERR(priv->dmabuf_priv)) {
+   ret = PTR_ERR(priv->dmabuf_priv);
+   kfree(priv);
+   return ret;
+   }
+#endif
+
if (use_ptemod) {
priv->mm = get_task_mm(current);
if (!priv->mm) {
@@ -616,8 +628,13 @@ static int gntdev_release(struct inode *inode, struct file 
*flip)
WARN_ON(!list_empty(>freeable_maps));
mutex_unlock(>lock);
  
+#ifdef CONFIG_XEN_GNTDEV_DMABUF

+   gntdev_dmabuf_fini(priv->dmabuf_priv);
+#endif
+
if (use_ptemod)
mmu_notifier_unregister(>mn, priv->mm);
+
kfree(priv);
return 0;
  }
@@ -987,6 +1004,107 @@ static long gntdev_ioctl_grant_copy(struct gntdev_priv 
*priv, void __user *u)
return ret;
  }
  
+#ifdef CONFIG_XEN_GNTDEV_DMABUF

+static long
+gntdev_ioctl_dmabuf_exp_from_refs(struct gntdev_priv *priv,
+ struct ioctl_gntdev_dmabuf_exp_from_refs 
__user *u)



Didn't we agree that this code moves to gntdev-dmabuf.c ?

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v2 5/9] xen/gntdev: Allow mappings for DMA buffers

[Boris Ostrovsky]

On 06/08/2018 01:59 PM, Stefano Stabellini wrote:
>
     @@ -325,6 +401,14 @@ static int map_grant_pages(struct
 grant_map
 *map)
     map->unmap_ops[i].handle = map->map_ops[i].handle;
     if (use_ptemod)
     map->kunmap_ops[i].handle =
 map->kmap_ops[i].handle;
 +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
 +    else if (map->dma_vaddr) {
 +    unsigned long mfn;
 +
 +    mfn = __pfn_to_mfn(page_to_pfn(map->pages[i]));
>>> Not pfn_to_mfn()?
>> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1]
>> and [2]
>> Thus,
>>
>> drivers/xen/gntdev.c:408:10: error: implicit declaration of function
>> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration]
>>   mfn = pfn_to_mfn(page_to_pfn(map->pages[i]));
>>
>> So, I'll keep __pfn_to_mfn
> How will this work on non-PV x86?
 So, you mean I need:
 #ifdef CONFIG_X86
 mfn = pfn_to_mfn(page_to_pfn(map->pages[i]));
 #else
 mfn = __pfn_to_mfn(page_to_pfn(map->pages[i]));
 #endif

>>> I'd rather fix it in ARM code. Stefano, why does ARM uses the
>>> underscored version?
>> Do you want me to add one more patch for ARM to wrap __pfn_to_mfn
>> with static inline for ARM? e.g.
>> static inline ...pfn_to_mfn(...)
>> {
>>     __pfn_to_mfn();
>> }
>
> A Xen on ARM guest doesn't actually know the mfns behind its own
> pseudo-physical pages. This is why we stopped using pfn_to_mfn and
> started using pfn_to_bfn instead, which will generally return "pfn",
> unless the page is a foreign grant. See include/xen/arm/page.h.
> pfn_to_bfn was also introduced on x86. For example, see the usage of
> pfn_to_bfn in drivers/xen/swiotlb-xen.c. Otherwise, if you don't care
> about other mapped grants, you can just use pfn_to_gfn, that always
> returns pfn.


I think then this code needs to use pfn_to_bfn().



>
> Also, for your information, we support different page granularities in
> Linux as a Xen guest, see the comment at include/xen/arm/page.h:
>
>   /*
>* The pseudo-physical frame (pfn) used in all the helpers is always based
>* on Xen page granularity (i.e 4KB).
>*
>* A Linux page may be split across multiple non-contiguous Xen page so we
>* have to keep track with frame based on 4KB page granularity.
>*
>* PV drivers should never make a direct usage of those helpers 
> (particularly
>* pfn_to_gfn and gfn_to_pfn).
>*/
>
> A Linux page could be 64K, but a Xen page is always 4K. A granted page
> is also 4K. We have helpers to take into account the offsets to map
> multiple Xen grants in a single Linux page, see for example
> drivers/xen/grant-table.c:gnttab_foreach_grant. Most PV drivers have
> been converted to be able to work with 64K pages correctly, but if I
> remember correctly gntdev.c is the only remaining driver that doesn't
> support 64K pages yet, so you don't have to deal with it if you don't
> want to.


I believe somewhere in this series there is a test for PAGE_SIZE vs.
XEN_PAGE_SIZE. Right, Oleksandr?

Thanks for the explanation.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v2 5/9] xen/gntdev: Allow mappings for DMA buffers

[Boris Ostrovsky]

(Stefano, question for you at the end)

On 06/07/2018 02:39 AM, Oleksandr Andrushchenko wrote:
> On 06/07/2018 12:19 AM, Boris Ostrovsky wrote:
>> On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote:
>>> On 06/04/2018 11:12 PM, Boris Ostrovsky wrote:
>>>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
>>>> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map
>>>> *map)
>>>>    if (map == NULL)
>>>>    return;
>>>>    +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> *Option 1: kfree(map->frames);*
>>>> +    if (map->dma_vaddr) {
>>>> +    struct gnttab_dma_alloc_args args;
>>>> +
>>>> +    args.dev = map->dma_dev;
>>>> +    args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT;
>>>> +    args.nr_pages = map->count;
>>>> +    args.pages = map->pages;
>>>> +    args.frames = map->frames;
>>>> +    args.vaddr = map->dma_vaddr;
>>>> +    args.dev_bus_addr = map->dma_bus_addr;
>>>> +
>>>> +    gnttab_dma_free_pages();
> *Option 2: kfree(map->frames);*
>>>> +    } else
>>>> +#endif
>>>>    if (map->pages)
>>>>    gnttab_free_pages(map->count, map->pages);
>>>> +
>>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>>>> +    kfree(map->frames);
>>>> +#endif
>>>>
>>>> Can this be done under if (map->dma_vaddr) ?
>>>>    In other words, is it
>>>> possible for dma_vaddr to be NULL and still have unallocated frames
>>>> pointer?
>>> It is possible to have vaddr == NULL and frames != NULL as we
>>> allocate frames outside of gnttab_dma_alloc_pages which
>>> may fail. Calling kfree on NULL pointer is safe,
>>
>> I am not questioning safety of the code, I would like avoid another
>> ifdef.
> Ah, I now understand, so you are asking if we can have
> that kfree(map->frames); in the place *Option 2* I marked above.
> Unfortunately no: map->frames is allocated before we try to
> allocate DMA memory, e.g. before dma_vaddr is set:
> [...]
>         add->frames = kcalloc(count, sizeof(add->frames[0]),
>                   GFP_KERNEL);
>         if (!add->frames)
>             goto err;
>
> [...]
>         if (gnttab_dma_alloc_pages())
>             goto err;
>
>         add->dma_vaddr = args.vaddr;
> [...]
> err:
>     gntdev_free_map(add);
>
> So, it is possible to enter gntdev_free_map with
> frames != NULL and dma_vaddr == NULL. Option 1 above cannot be used
> as map->frames is needed for gnttab_dma_free_pages();
> and Option 2 cannot be used as frames != NULL and dma_vaddr == NULL.
> Thus, I think that unfortunately we need that #ifdef.
> Option 3 below can also be considered, but that seems to be not good
> as we free resources in different places which looks inconsistent.


I was only thinking of option 2. But if it is possible to have frames !=
NULL and dma_vaddr == NULL then perhaps we indeed will have to live with
the extra ifdef.


>
> Sorry if I'm still missing your point.
>>
>>> so
>>> I see no reason to change this code.
>>>>>    kfree(map->pages);
>>>>>    kfree(map->grants);
>>>>>    kfree(map->map_ops);
>>>>> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map
>>>>> *map)
>>>>>    kfree(map);
>>>>>    }
>>>>>    -static struct grant_map *gntdev_alloc_map(struct gntdev_priv
>>>>> *priv, int count)
>>>>> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv,
>>>>> int count,
>>>>> +  int dma_flags)
>>>>>    {
>>>>>    struct grant_map *add;
>>>>>    int i;
>>>>> @@ -155,6 +200,37 @@ static struct grant_map
>>>>> *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>>>>>    NULL == add->pages)
>>>>>    goto err;
>>>>>    +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>>>>> +    add->dma_flags = dma_flags;
>>>>> +
>>>>> +    /*
>>>>> + * Check if this mapping is requested to be backed
>>>>> + * by a DMA buffer.
>>>>> + */
>>>>> +    if (dma_flags & (GNTDEV_DMA_FLAG_WC |
>>>>> GNTDEV_DMA_FLAG_COHERENT)) {
>>>>> 

Re: [PATCH v2 7/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

On 06/07/2018 04:44 AM, Oleksandr Andrushchenko wrote:
> On 06/07/2018 12:48 AM, Boris Ostrovsky wrote:
>> On 06/06/2018 08:10 AM, Oleksandr Andrushchenko wrote:
>>> On 06/05/2018 01:07 AM, Boris Ostrovsky wrote:
>>>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
>>
>>>> +
>>>> +static struct sg_table *
>>>> +dmabuf_exp_ops_map_dma_buf(struct dma_buf_attachment *attach,
>>>> +   enum dma_data_direction dir)
>>>> +{
>>>> +    struct gntdev_dmabuf_attachment *gntdev_dmabuf_attach =
>>>> attach->priv;
>>>> +    struct gntdev_dmabuf *gntdev_dmabuf = attach->dmabuf->priv;
>>>> +    struct sg_table *sgt;
>>>> +
>>>> +    pr_debug("Mapping %d pages for dev %p\n",
>>>> gntdev_dmabuf->nr_pages,
>>>> + attach->dev);
>>>> +
>>>> +    if (WARN_ON(dir == DMA_NONE || !gntdev_dmabuf_attach))
>>>>
>>>> WARN_ON_ONCE. Here and elsewhere.
>>> Why? The UAPI may be used by different applications, thus we might
>>> lose warnings for some of them. Having WARN_ON will show problems
>>> for multiple users, not for the first one.
>>> Does this make sense to still use WARN_ON?
>>
>> Just as with pr_err call somewhere else the concern here is that
>> userland (which I think is where this is eventually called from?) may
>> intentionally trigger the error, flooding the log.
>>
>> And even this is not directly called from userland there is still a
>> possibility of triggering this error multiple times.
> Ok, will use WARN_ON_ONCE


In fact, is there a reason to use WARN at all? Does this condition
indicate some sort of internal inconsistency/error?

-boris



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/9] xen/gntdev: Add initial support for dma-buf UAPI

[Boris Ostrovsky]

On 06/07/2018 03:17 AM, Oleksandr Andrushchenko wrote:
> On 06/07/2018 12:32 AM, Boris Ostrovsky wrote:
>> On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote:
>>> On 06/04/2018 11:49 PM, Boris Ostrovsky wrote:
>>>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>>>>> index 9813fc440c70..7d58dfb3e5e8 100644
>>>>> --- a/drivers/xen/gntdev.c
>>>>> +++ b/drivers/xen/gntdev.c
>>>> ...
>>>>
>>>>>    +#ifdef CONFIG_XEN_GNTDEV_DMABUF
>>>> This code belongs in gntdev-dmabuf.c.
>>> The reason I have this code here is that it is heavily
>>> tied to gntdev's internal functionality, e.g. map/unmap.
>>> I do not want to extend gntdev's API, so gntdev-dmabuf can
>>> access these. What is more dma-buf doesn't need to know about
>>> maps done by gntdev as there is no use of that information
>>> in gntdev-dmabuf. So, it seems more naturally to have
>>> dma-buf's related map/unmap code where it is: in gntdev.
>> Sorry, I don't follow. Why would this require extending the API? It's
>> just moving routines to a different file that is linked to the same
>> module.
> I do understand your intention here and tried to avoid dma-buf
> related code in gntdev.c as much as possible. So, let me explain
> my decision in more detail.
>
> There are 2 use-cases we have: dma-buf import and export.
>
> While importing a dma-buf all the dma-buf related functionality can
> easily be kept inside gntdev-dmabuf.c w/o any issue as all we need
> from gntdev.c is dev, dma_buf_fd, count and domid for that.
>
> But in case of dma-buf export we need to:
> 1. struct grant_map *map = gntdev_alloc_map(priv, count, dmabuf_flags);
> 2. gntdev_add_map(priv, map);
> 3. Set map->flags
> 4. ret = map_grant_pages(map);
> 5. And only now we are all set to export the new dma-buf from
> *map->pages*
>
> So, until 5) we use private gtndev.c's API not exported to outside world:
> a. struct grant_map
> b. static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv,
> int count,
>                       int dma_flags)
> c. static void gntdev_add_map(struct gntdev_priv *priv, struct
> grant_map *add)
> d. static int map_grant_pages(struct grant_map *map)
>
> Thus, all the above cannot be accessed from gntdev-dmabuf.c
> This is why I say that gntdev.c's API will need to be extended to
> provide the above
> a-d if we want all dma-buf export code to leave in gntdev-dmabuf.c.



I still don't understand why you feel this would be extending the API.
These routines and the struct can be declared in local header file and
this header file will not be visible to anyone but gntdev.c and
gntdev-dmabuf.c. You can, for example, put this into gntdev-dmabuf.h
(and then rename it to something else, like gntdev-common.h).



> But that doesn't seem good to me and what is more a-d are really
> gntdev.c's
> functionality, not dma-buf's which only needs pages and doesn't really
> care from
> where those come.
> That was the reason I partitioned export into 2 chunks: gntdev +
> gntdev-dmabuf.
>
> You might also ask why importing side does Xen related things
> (granting references+)
> in gntdev-dmabuf, not gntdev so it is consistent with the dma-buf
> exporter?
> This is because importer uses grant-table's API which seems to be not
> natural for gntdev.c,
> so it can leave in gntdev-dmabuf.c which has a use-case for that,
> while gntdev
> remains the same.


Yet another reason why this code should be moved: importing and
exporting functionalities logically belong together. The fat that they
are implemented using different methods is not relevant IMO.

If you have code which is under ifdef CONFIG_GNTDEV_DMABUF and you have
file called gntdev-dmabuf.c it sort of implies that this code should
live in that file (unless that code is intertwined with other code,
which is not the case here).


-boris



>> Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in
>> gntdev-dmabuf.c? In my view that's the file where all dma-related
>> "stuff" lives.
> Agree, but IMO grant_map stuff for dma-buf importer is right in its
> place in gntdev.c
> and all the rest of dma-buf specifics live in gntdev-dmabuf.c as they
> should
>>
>> -boris
>>
>>
>> -boris
>>
> Thank you,
> Oleksandr

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/9] xen/balloon: Share common memory reservation routines

[Boris Ostrovsky]

On 06/06/2018 03:24 AM, Oleksandr Andrushchenko wrote:
> On 06/04/2018 07:37 PM, Boris Ostrovsky wrote:
>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
>>> diff --git a/include/xen/mem-reservation.h
>>> b/include/xen/mem-reservation.h
>>> new file mode 100644
>>> index ..a727d65a1e61
>>> --- /dev/null
>>> +++ b/include/xen/mem-reservation.h
>>> @@ -0,0 +1,65 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>> +/*
>>> + * Xen memory reservation utilities.
>>> + *
>>> + * Copyright (c) 2003, B Dragovic
>>> + * Copyright (c) 2003-2004, M Williamson, K Fraser
>>> + * Copyright (c) 2005 Dan M. Smith, IBM Corporation
>>> + * Copyright (c) 2010 Daniel Kiper
>>> + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
>>> + */
>>> +
>>> +#ifndef _XENMEM_RESERVATION_H
>>> +#define _XENMEM_RESERVATION_H
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +#ifdef CONFIG_XEN_SCRUB_PAGES
>>> +void xenmem_reservation_scrub_page(struct page *page);
>>> +#else
>>> +static inline void xenmem_reservation_scrub_page(struct page *page)
>>> +{
>>> +}
>>> +#endif
>>
>> Given that this is a wrapper around a single call I'd prefer
>>
>> inline void xenmem_reservation_scrub_page(struct page *page)
>> {
>> #ifdef CONFIG_XEN_SCRUB_PAGES
>>  clear_highpage(page);
>> #endif
>> }
> Unfortunately this can't be done because of
> EXPORT_SYMBOL_GPL(xenmem_reservation_scrub_page);
> which is obviously cannot be used for static inline functions.



Why do you need to export it? It's an inline defined in the header file.
Just like clear_highpage().


-boris

> So, I'll keep it as is.
>>
>>
>> -boris
>>
>>
> Thank you,
> Oleksandr

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v2 9/9] xen/gntdev: Expose gntdev's dma-buf API for in-kernel use

[Boris Ostrovsky]

On 06/06/2018 08:46 AM, Oleksandr Andrushchenko wrote:
> On 06/05/2018 01:36 AM, Boris Ostrovsky wrote:
>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko 
>>>
>>> Allow creating grant device context for use by kernel modules which
>>> require functionality, provided by gntdev. Export symbols for dma-buf
>>> API provided by the module.
>> Can you give an example of who'd be using these interfaces?
> There is no use-case at the moment I can think of, but hyper dma-buf
> [1], [2]
> I let Intel folks (CCed) to defend this patch as it was done primarily
> for them
> and I don't use it in any of my use-cases. So, from this POV it can be
> dropped,
> at least from this series.


Yes, let's drop this until someone actually needs it.

-boris


>>
>> -boris
>>
> [1] https://patchwork.freedesktop.org/series/38207/
> [2] https://patchwork.freedesktop.org/patch/204447/
>
> ___
> Xen-devel mailing list
> xen-de...@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 6/9] xen/gntdev: Add initial support for dma-buf UAPI

[Boris Ostrovsky]

On 06/06/2018 05:06 AM, Oleksandr Andrushchenko wrote:
> On 06/04/2018 11:49 PM, Boris Ostrovsky wrote:
>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:

>> +struct gntdev_dmabuf_export_args {
>> +    int dummy;
>> +};
>>
>> Please define the full structure (at least what you have in the next
>> patch) here.
> Ok, will define what I have in the next patch, but won't
> initialize anything until the next patch. Will this work for you?

Sure, I just didn't see the need for the dummy argument that you remove
later.

>>
>>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>>> index 9813fc440c70..7d58dfb3e5e8 100644
>>> --- a/drivers/xen/gntdev.c
>>> +++ b/drivers/xen/gntdev.c
>> ...
>>
>>>   +#ifdef CONFIG_XEN_GNTDEV_DMABUF
>> This code belongs in gntdev-dmabuf.c.
> The reason I have this code here is that it is heavily
> tied to gntdev's internal functionality, e.g. map/unmap.
> I do not want to extend gntdev's API, so gntdev-dmabuf can
> access these. What is more dma-buf doesn't need to know about
> maps done by gntdev as there is no use of that information
> in gntdev-dmabuf. So, it seems more naturally to have
> dma-buf's related map/unmap code where it is: in gntdev.

Sorry, I don't follow. Why would this require extending the API? It's
just moving routines to a different file that is linked to the same module.

Since this is under CONFIG_XEN_GNTDEV_DMABUF then why shouldn't it be in
gntdev-dmabuf.c? In my view that's the file where all dma-related
"stuff" lives.


-boris


-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

On 06/06/2018 08:10 AM, Oleksandr Andrushchenko wrote:
> On 06/05/2018 01:07 AM, Boris Ostrovsky wrote:
>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:


>> +
>> +static struct sg_table *
>> +dmabuf_exp_ops_map_dma_buf(struct dma_buf_attachment *attach,
>> +   enum dma_data_direction dir)
>> +{
>> +    struct gntdev_dmabuf_attachment *gntdev_dmabuf_attach =
>> attach->priv;
>> +    struct gntdev_dmabuf *gntdev_dmabuf = attach->dmabuf->priv;
>> +    struct sg_table *sgt;
>> +
>> +    pr_debug("Mapping %d pages for dev %p\n", gntdev_dmabuf->nr_pages,
>> + attach->dev);
>> +
>> +    if (WARN_ON(dir == DMA_NONE || !gntdev_dmabuf_attach))
>>
>> WARN_ON_ONCE. Here and elsewhere.
> Why? The UAPI may be used by different applications, thus we might
> lose warnings for some of them. Having WARN_ON will show problems
> for multiple users, not for the first one.
> Does this make sense to still use WARN_ON?


Just as with pr_err call somewhere else the concern here is that
userland (which I think is where this is eventually called from?) may
intentionally trigger the error, flooding the log.

And even this is not directly called from userland there is still a
possibility of triggering this error multiple times.


>>
>>> +
>>> +    if (use_ptemod) {
>>> +    pr_err("Cannot provide dma-buf: use_ptemode %d\n",
>>> +   use_ptemod);
>> No pr_err here please. This can potentially become a DoS vector as it
>> comes directly from ioctl.
>>
>> I would, in fact, revisit other uses of pr_err in this file.
> Sure, all of pr_err can actually be pr_debug...

I'd check even further and see if any prink is needed. I think I saw a
couple that were not especially useful.


>>> +    return -EINVAL;
>>> +    }
>>> +
>>> +    map = dmabuf_exp_alloc_backing_storage(priv, flags, count);
>>
>> @count comes from userspace. dmabuf_exp_alloc_backing_storage only
>> checks for it to be >0. Should it be checked for some sane max value?
> This is not easy as it is hard to tell what could be that
> max value. For DMA buffers if count is too big then allocation
> will fail, so need to check for max here  (dma_alloc_{xxx} will
> filter out too big allocations).

OK, that may be sufficient. BTW, I believe there were other loops with
@count being the control variable. Please see if a user can pass a bogus
value.

> For Xen balloon allocations I cannot tell what could be that
> max value neither. Tough question how to limit.

I think in balloon there is also a guarantee (of sorts) that something
prior to a loop will fail.


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v2 5/9] xen/gntdev: Allow mappings for DMA buffers

[Boris Ostrovsky]

On 06/06/2018 04:14 AM, Oleksandr Andrushchenko wrote:
> On 06/04/2018 11:12 PM, Boris Ostrovsky wrote:
>> On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:

>> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map)
>>   if (map == NULL)
>>   return;
>>   +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>> +    if (map->dma_vaddr) {
>> +    struct gnttab_dma_alloc_args args;
>> +
>> +    args.dev = map->dma_dev;
>> +    args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT;
>> +    args.nr_pages = map->count;
>> +    args.pages = map->pages;
>> +    args.frames = map->frames;
>> +    args.vaddr = map->dma_vaddr;
>> +    args.dev_bus_addr = map->dma_bus_addr;
>> +
>> +    gnttab_dma_free_pages();
>> +    } else
>> +#endif
>>   if (map->pages)
>>   gnttab_free_pages(map->count, map->pages);
>> +
>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>> +    kfree(map->frames);
>> +#endif
>>
>> Can this be done under if (map->dma_vaddr) ?
>
>>   In other words, is it
>> possible for dma_vaddr to be NULL and still have unallocated frames
>> pointer?
> It is possible to have vaddr == NULL and frames != NULL as we
> allocate frames outside of gnttab_dma_alloc_pages which
> may fail. Calling kfree on NULL pointer is safe,


I am not questioning safety of the code, I would like avoid another ifdef.


> so
> I see no reason to change this code.
>>
>>>   kfree(map->pages);
>>>   kfree(map->grants);
>>>   kfree(map->map_ops);
>>> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map)
>>>   kfree(map);
>>>   }
>>>   -static struct grant_map *gntdev_alloc_map(struct gntdev_priv
>>> *priv, int count)
>>> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv,
>>> int count,
>>> +  int dma_flags)
>>>   {
>>>   struct grant_map *add;
>>>   int i;
>>> @@ -155,6 +200,37 @@ static struct grant_map
>>> *gntdev_alloc_map(struct gntdev_priv *priv, int count)
>>>   NULL == add->pages)
>>>   goto err;
>>>   +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>>> +    add->dma_flags = dma_flags;
>>> +
>>> +    /*
>>> + * Check if this mapping is requested to be backed
>>> + * by a DMA buffer.
>>> + */
>>> +    if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) {
>>> +    struct gnttab_dma_alloc_args args;
>>> +
>>> +    add->frames = kcalloc(count, sizeof(add->frames[0]),
>>> +  GFP_KERNEL);
>>> +    if (!add->frames)
>>> +    goto err;
>>> +
>>> +    /* Remember the device, so we can free DMA memory. */
>>> +    add->dma_dev = priv->dma_dev;
>>> +
>>> +    args.dev = priv->dma_dev;
>>> +    args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT;
>>> +    args.nr_pages = count;
>>> +    args.pages = add->pages;
>>> +    args.frames = add->frames;
>>> +
>>> +    if (gnttab_dma_alloc_pages())
>>> +    goto err;
>>> +
>>> +    add->dma_vaddr = args.vaddr;
>>> +    add->dma_bus_addr = args.dev_bus_addr;
>>> +    } else
>>> +#endif
>>>   if (gnttab_alloc_pages(count, add->pages))
>>>   goto err;
>>>   @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map
>>> *map)
>>>   map->unmap_ops[i].handle = map->map_ops[i].handle;
>>>   if (use_ptemod)
>>>   map->kunmap_ops[i].handle = map->kmap_ops[i].handle;
>>> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>>> +    else if (map->dma_vaddr) {
>>> +    unsigned long mfn;
>>> +
>>> +    mfn = __pfn_to_mfn(page_to_pfn(map->pages[i]));
>>
>> Not pfn_to_mfn()?
> I'd love to, but pfn_to_mfn is only defined for x86, not ARM: [1] and [2]
> Thus,
>
> drivers/xen/gntdev.c:408:10: error: implicit declaration of function
> ‘pfn_to_mfn’ [-Werror=implicit-function-declaration]
>     mfn = pfn_to_mfn(page_to_pfn(map->pages[i]));
>
> So, I'll keep __pfn_to_mfn


How will this work on non-PV x86?

-boris


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/9] xen/gntdev: Allow mappings for DMA buffers

[Boris Ostrovsky]

On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Allow mappings for DMA backed  buffers if grant table module
> supports such: this extends grant device to not only map buffers
> made of balloon pages, but also from buffers allocated with
> dma_alloc_xxx.
>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/gntdev.c  | 99 ++-
>  include/uapi/xen/gntdev.h | 15 ++
>  2 files changed, 112 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index bd56653b9bbc..9813fc440c70 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -37,6 +37,9 @@
>  #include 
>  #include 
>  #include 
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> +#include 
> +#endif
>  
>  #include 
>  #include 
> @@ -72,6 +75,11 @@ struct gntdev_priv {
>   struct mutex lock;
>   struct mm_struct *mm;
>   struct mmu_notifier mn;
> +
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + /* Device for which DMA memory is allocated. */
> + struct device *dma_dev;
> +#endif
>  };
>  
>  struct unmap_notify {
> @@ -96,10 +104,27 @@ struct grant_map {
>   struct gnttab_unmap_grant_ref *kunmap_ops;
>   struct page **pages;
>   unsigned long pages_vm_start;
> +
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + /*
> +  * If dmabuf_vaddr is not NULL then this mapping is backed by DMA
> +  * capable memory.
> +  */
> +
> + struct device *dma_dev;
> + /* Flags used to create this DMA buffer: GNTDEV_DMA_FLAG_XXX. */
> + int dma_flags;
> + void *dma_vaddr;
> + dma_addr_t dma_bus_addr;
> + /* This is required for gnttab_dma_{alloc|free}_pages. */

How about

/* Needed to avoid allocation in gnttab_dma_free_pages(). */

> + xen_pfn_t *frames;
> +#endif
>  };
>  
>  static int unmap_grant_pages(struct grant_map *map, int offset, int pages);
>  
> +static struct miscdevice gntdev_miscdev;
> +
>  /* -- */
>  
>  static void gntdev_print_maps(struct gntdev_priv *priv,
> @@ -121,8 +146,27 @@ static void gntdev_free_map(struct grant_map *map)
>   if (map == NULL)
>   return;
>  
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + if (map->dma_vaddr) {
> + struct gnttab_dma_alloc_args args;
> +
> + args.dev = map->dma_dev;
> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT;
> + args.nr_pages = map->count;
> + args.pages = map->pages;
> + args.frames = map->frames;
> + args.vaddr = map->dma_vaddr;
> + args.dev_bus_addr = map->dma_bus_addr;
> +
> + gnttab_dma_free_pages();
> + } else
> +#endif
>   if (map->pages)
>   gnttab_free_pages(map->count, map->pages);
> +
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + kfree(map->frames);
> +#endif


Can this be done under if (map->dma_vaddr) ? In other words, is it
possible for dma_vaddr to be NULL and still have unallocated frames pointer?


>   kfree(map->pages);
>   kfree(map->grants);
>   kfree(map->map_ops);
> @@ -132,7 +176,8 @@ static void gntdev_free_map(struct grant_map *map)
>   kfree(map);
>  }
>  
> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int 
> count)
> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int 
> count,
> +   int dma_flags)
>  {
>   struct grant_map *add;
>   int i;
> @@ -155,6 +200,37 @@ static struct grant_map *gntdev_alloc_map(struct 
> gntdev_priv *priv, int count)
>   NULL == add->pages)
>   goto err;
>  
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + add->dma_flags = dma_flags;
> +
> + /*
> +  * Check if this mapping is requested to be backed
> +  * by a DMA buffer.
> +  */
> + if (dma_flags & (GNTDEV_DMA_FLAG_WC | GNTDEV_DMA_FLAG_COHERENT)) {
> + struct gnttab_dma_alloc_args args;
> +
> + add->frames = kcalloc(count, sizeof(add->frames[0]),
> +   GFP_KERNEL);
> + if (!add->frames)
> + goto err;
> +
> + /* Remember the device, so we can free DMA memory. */
> + add->dma_dev = priv->dma_dev;
> +
> + args.dev = priv->dma_dev;
> + args.coherent = dma_flags & GNTDEV_DMA_FLAG_COHERENT;
> + args.nr_pages = count;
> + args.pages = add->pages;
> + args.frames = add->frames;
> +
> + if (gnttab_dma_alloc_pages())
> + goto err;
> +
> + add->dma_vaddr = args.vaddr;
> + add->dma_bus_addr = args.dev_bus_addr;
> + } else
> +#endif
>   if (gnttab_alloc_pages(count, add->pages))
>   goto err;
>  
> @@ -325,6 +401,14 @@ static int map_grant_pages(struct grant_map *map)
>   map->unmap_ops[i].handle = 

Re: [PATCH v2 8/9] xen/gntdev: Implement dma-buf import functionality

[Boris Ostrovsky]

On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
>  /* -- */
>  
> +static int
> +dmabuf_imp_grant_foreign_access(struct page **pages, u32 *refs,
> + int count, int domid)
> +{
> + grant_ref_t priv_gref_head;
> + int i, ret;
> +
> + ret = gnttab_alloc_grant_references(count, _gref_head);
> + if (ret < 0) {
> + pr_err("Cannot allocate grant references, ret %d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < count; i++) {
> + int cur_ref;
> +
> + cur_ref = gnttab_claim_grant_reference(_gref_head);
> + if (cur_ref < 0) {
> + ret = cur_ref;
> + pr_err("Cannot claim grant reference, ret %d\n", ret);
> + goto out;
> + }
> +
> + gnttab_grant_foreign_access_ref(cur_ref, domid,
> + xen_page_to_gfn(pages[i]), 0);
> + refs[i] = cur_ref;
> + }
> +
> + ret = 0;

return 0?

> +
> +out:
> + gnttab_free_grant_references(priv_gref_head);
> + return ret;
> +}
> +

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 9/9] xen/gntdev: Expose gntdev's dma-buf API for in-kernel use

[Boris Ostrovsky]

On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Allow creating grant device context for use by kernel modules which
> require functionality, provided by gntdev. Export symbols for dma-buf
> API provided by the module.

Can you give an example of who'd be using these interfaces?


-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 7/9] xen/gntdev: Implement dma-buf export functionality

[Boris Ostrovsky]

On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> 1. Create a dma-buf from grant references provided by the foreign
>domain. By default dma-buf is backed by system memory pages, but
>by providing GNTDEV_DMA_FLAG_XXX flags it can also be created
>as a DMA write-combine/coherent buffer, e.g. allocated with
>corresponding dma_alloc_xxx API.
>Export the resulting buffer as a new dma-buf.
>
> 2. Implement waiting for the dma-buf to be released: block until the
>dma-buf with the file descriptor provided is released.
>If within the time-out provided the buffer is not released then
>-ETIMEDOUT error is returned. If the buffer with the file descriptor
>does not exist or has already been released, then -ENOENT is
>returned. For valid file descriptors this must not be treated as
>error.
>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/gntdev-dmabuf.c | 393 +++-
>  drivers/xen/gntdev-dmabuf.h |   9 +-
>  drivers/xen/gntdev.c|  90 -
>  3 files changed, 486 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 6bedd1387bd9..f612468879b4 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -3,15 +3,58 @@
>  /*
>   * Xen dma-buf functionality for gntdev.
>   *
> + * DMA buffer implementation is based on drivers/gpu/drm/drm_prime.c.
> + *
>   * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
>   */
>  
> +#include 
>  #include 
>  
>  #include "gntdev-dmabuf.h"
>  
> +struct gntdev_dmabuf {
> + struct gntdev_dmabuf_priv *priv;
> + struct dma_buf *dmabuf;
> + struct list_head next;
> + int fd;
> +
> + union {
> + struct {
> + /* Exported buffers are reference counted. */
> + struct kref refcount;
> +
> + struct gntdev_priv *priv;
> + struct grant_map *map;
> + void (*release)(struct gntdev_priv *priv,
> + struct grant_map *map);
> + } exp;
> + } u;
> +
> + /* Number of pages this buffer has. */
> + int nr_pages;
> + /* Pages of this buffer. */
> + struct page **pages;
> +};
> +
> +struct gntdev_dmabuf_wait_obj {
> + struct list_head next;
> + struct gntdev_dmabuf *gntdev_dmabuf;
> + struct completion completion;
> +};
> +
> +struct gntdev_dmabuf_attachment {
> + struct sg_table *sgt;
> + enum dma_data_direction dir;
> +};
> +
>  struct gntdev_dmabuf_priv {
> - int dummy;
> + /* List of exported DMA buffers. */
> + struct list_head exp_list;
> + /* List of wait objects. */
> + struct list_head exp_wait_list;
> + /* This is the lock which protects dma_buf_xxx lists. */
> + struct mutex lock;
>  };
>  
>  /* -- */
> @@ -22,19 +65,359 @@ struct gntdev_dmabuf_priv {
>  /* Implementation of wait for exported DMA buffer to be released. */
>  /* -- */
>  
> +static void dmabuf_exp_release(struct kref *kref);
> +
> +static struct gntdev_dmabuf_wait_obj *
> +dmabuf_exp_wait_obj_new(struct gntdev_dmabuf_priv *priv,
> + struct gntdev_dmabuf *gntdev_dmabuf)
> +{
> + struct gntdev_dmabuf_wait_obj *obj;
> +
> + obj = kzalloc(sizeof(*obj), GFP_KERNEL);
> + if (!obj)
> + return ERR_PTR(-ENOMEM);
> +
> + init_completion(>completion);
> + obj->gntdev_dmabuf = gntdev_dmabuf;
> +
> + mutex_lock(>lock);
> + list_add(>next, >exp_wait_list);
> + /* Put our reference and wait for gntdev_dmabuf's release to fire. */
> + kref_put(_dmabuf->u.exp.refcount, dmabuf_exp_release);
> + mutex_unlock(>lock);
> + return obj;
> +}
> +
> +static void dmabuf_exp_wait_obj_free(struct gntdev_dmabuf_priv *priv,
> +  struct gntdev_dmabuf_wait_obj *obj)
> +{
> + struct gntdev_dmabuf_wait_obj *cur_obj, *q;
> +
> + mutex_lock(>lock);
> + list_for_each_entry_safe(cur_obj, q, >exp_wait_list, next)
> + if (cur_obj == obj) {
> + list_del(>next);
> + kfree(obj);
> + break;
> + }
> + mutex_unlock(>lock);
> +}
> +
> +static int dmabuf_exp_wait_obj_wait(struct gntdev_dmabuf_wait_obj *obj,
> + u32 wait_to_ms)
> +{
> + if (wait_for_completion_timeout(>completion,
> + msecs_to_jiffies(wait_to_ms)) <= 0)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +static void dmabuf_exp_wait_obj_signal(struct gntdev_dmabuf_priv *priv,
> +struct gntdev_dmabuf *gntdev_dmabuf)
> +{
> + struct gntdev_dmabuf_wait_obj *obj, *q;
> +
> + 

Re: [PATCH v2 4/9] xen/grant-table: Allow allocating buffers suitable for DMA

[Boris Ostrovsky]

On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Extend grant table module API to allow allocating buffers that can
> be used for DMA operations and mapping foreign grant references
> on top of those.
> The resulting buffer is similar to the one allocated by the balloon
> driver in terms that proper memory reservation is made
> ({increase|decrease}_reservation and VA mappings updated if needed).
> This is useful for sharing foreign buffers with HW drivers which
> cannot work with scattered buffers provided by the balloon driver,
> but require DMAable memory instead.
>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/Kconfig   |  13 +
>  drivers/xen/grant-table.c | 109 ++
>  include/xen/grant_table.h |  18 +++
>  3 files changed, 140 insertions(+)
>
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index e5d0c28372ea..39536ddfbce4 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -161,6 +161,19 @@ config XEN_GRANT_DEV_ALLOC
> to other domains. This can be used to implement frontend drivers
> or as part of an inter-domain shared memory channel.
>  
> +config XEN_GRANT_DMA_ALLOC
> + bool "Allow allocating DMA capable buffers with grant reference module"
> + depends on XEN && HAS_DMA
> + help
> +   Extends grant table module API to allow allocating DMA capable
> +   buffers and mapping foreign grant references on top of it.
> +   The resulting buffer is similar to one allocated by the balloon
> +   driver in terms that proper memory reservation is made
> +   ({increase|decrease}_reservation and VA mappings updated if needed).
> +   This is useful for sharing foreign buffers with HW drivers which
> +   cannot work with scattered buffers provided by the balloon driver,
> +   but require DMAable memory instead.
> +
>  config SWIOTLB_XEN
>   def_bool y
>   select SWIOTLB
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index dbb48a89e987..5658e58d9cc6 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -45,6 +45,9 @@
>  #include 
>  #include 
>  #include 
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> +#include 
> +#endif
>  
>  #include 
>  #include 
> @@ -57,6 +60,7 @@
>  #ifdef CONFIG_X86
>  #include 
>  #endif
> +#include 
>  #include 
>  #include 
>  
> @@ -811,6 +815,73 @@ int gnttab_alloc_pages(int nr_pages, struct page **pages)
>  }
>  EXPORT_SYMBOL_GPL(gnttab_alloc_pages);
>  
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> +/**
> + * gnttab_dma_alloc_pages - alloc DMAable pages suitable for grant mapping 
> into
> + * @args: arguments to the function
> + */
> +int gnttab_dma_alloc_pages(struct gnttab_dma_alloc_args *args)
> +{
> + unsigned long pfn, start_pfn;
> + size_t size;
> + int i, ret;
> +
> + size = args->nr_pages << PAGE_SHIFT;
> + if (args->coherent)
> + args->vaddr = dma_alloc_coherent(args->dev, size,
> +  >dev_bus_addr,
> +  GFP_KERNEL | __GFP_NOWARN);
> + else
> + args->vaddr = dma_alloc_wc(args->dev, size,
> +>dev_bus_addr,
> +GFP_KERNEL | __GFP_NOWARN);
> + if (!args->vaddr) {
> + pr_err("Failed to allocate DMA buffer of size %zu\n", size);
> + return -ENOMEM;
> + }
> +
> + start_pfn = __phys_to_pfn(args->dev_bus_addr);
> + for (pfn = start_pfn, i = 0; pfn < start_pfn + args->nr_pages;
> + pfn++, i++) {
> + struct page *page = pfn_to_page(pfn);
> +
> + args->pages[i] = page;
> + args->frames[i] = xen_page_to_gfn(page);
> + xenmem_reservation_scrub_page(page);
> + }
> +
> + xenmem_reservation_va_mapping_reset(args->nr_pages, args->pages);
> +
> + ret = xenmem_reservation_decrease(args->nr_pages, args->frames);
> + if (ret != args->nr_pages) {
> + pr_err("Failed to decrease reservation for DMA buffer\n");
> + ret = -EFAULT;
> + goto fail_free_dma;
> + }
> +
> + ret = gnttab_pages_set_private(args->nr_pages, args->pages);
> + if (ret < 0)
> + goto fail_clear_private;
> +
> + return 0;
> +
> +fail_clear_private:
> + gnttab_pages_clear_private(args->nr_pages, args->pages);
> +fail_free_dma:
> + xenmem_reservation_increase(args->nr_pages, args->frames);
> + xenmem_reservation_va_mapping_update(args->nr_pages, args->pages,
> +  args->frames);
> + if (args->coherent)
> + dma_free_coherent(args->dev, size,
> +   args->vaddr, args->dev_bus_addr);
> + else
> + dma_free_wc(args->dev, size,
> + args->vaddr, args->dev_bus_addr);
> + return 

Re: [PATCH v2 6/9] xen/gntdev: Add initial support for dma-buf UAPI

[Boris Ostrovsky]

On 06/01/2018 07:41 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Add UAPI and IOCTLs for dma-buf grant device driver extension:
> the extension allows userspace processes and kernel modules to
> use Xen backed dma-buf implementation. With this extension grant
> references to the pages of an imported dma-buf can be exported
> for other domain use and grant references coming from a foreign
> domain can be converted into a local dma-buf for local export.
> Implement basic initialization and stubs for Xen DMA buffers'
> support.


It would be very helpful if people advocating for this interface
reviewed it as well.


>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/Kconfig |  10 +++
>  drivers/xen/Makefile|   1 +
>  drivers/xen/gntdev-dmabuf.c |  75 +++
>  drivers/xen/gntdev-dmabuf.h |  41 +++
>  drivers/xen/gntdev.c| 142 
>  include/uapi/xen/gntdev.h   |  91 +++
>  6 files changed, 360 insertions(+)
>  create mode 100644 drivers/xen/gntdev-dmabuf.c
>  create mode 100644 drivers/xen/gntdev-dmabuf.h
>
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index 39536ddfbce4..52d64e4b6b81 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -152,6 +152,16 @@ config XEN_GNTDEV
>   help
> Allows userspace processes to use grants.
>  
> +config XEN_GNTDEV_DMABUF
> + bool "Add support for dma-buf grant access device driver extension"
> + depends on XEN_GNTDEV && XEN_GRANT_DMA_ALLOC && DMA_SHARED_BUFFER


Is there a reason to have XEN_GRANT_DMA_ALLOC without XEN_GNTDEV_DMABUF?


> + help
> +   Allows userspace processes and kernel modules to use Xen backed
> +   dma-buf implementation. With this extension grant references to
> +   the pages of an imported dma-buf can be exported for other domain
> +   use and grant references coming from a foreign domain can be
> +   converted into a local dma-buf for local export.
> +
>  config XEN_GRANT_DEV_ALLOC
>   tristate "User-space grant reference allocator driver"
>   depends on XEN
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 3c87b0c3aca6..33afb7b2b227 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -41,5 +41,6 @@ obj-$(CONFIG_XEN_PVCALLS_BACKEND)   += pvcalls-back.o
>  obj-$(CONFIG_XEN_PVCALLS_FRONTEND)   += pvcalls-front.o
>  xen-evtchn-y := evtchn.o
>  xen-gntdev-y := gntdev.o
> +xen-gntdev-$(CONFIG_XEN_GNTDEV_DMABUF)   += gntdev-dmabuf.o
>  xen-gntalloc-y   := gntalloc.o
>  xen-privcmd-y:= privcmd.o
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> new file mode 100644
> index ..6bedd1387bd9
> --- /dev/null
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Xen dma-buf functionality for gntdev.
> + *
> + * Copyright (c) 2018 Oleksandr Andrushchenko, EPAM Systems Inc.
> + */
> +
> +#include 
> +
> +#include "gntdev-dmabuf.h"
> +
> +struct gntdev_dmabuf_priv {
> + int dummy;
> +};
> +
> +/* -- */
> +/* DMA buffer export support. */
> +/* -- */
> +
> +/* -- */
> +/* Implementation of wait for exported DMA buffer to be released. */
> +/* -- */

Why this comment style?

> +
> +int gntdev_dmabuf_exp_wait_released(struct gntdev_dmabuf_priv *priv, int fd,
> + int wait_to_ms)
> +{
> + return -EINVAL;
> +}
> +
> +/* -- */
> +/* DMA buffer export support. */
> +/* -- */
> +
> +int gntdev_dmabuf_exp_from_pages(struct gntdev_dmabuf_export_args *args)
> +{
> + return -EINVAL;
> +}
> +
> +/* -- */
> +/* DMA buffer import support. */
> +/* -- */
> +
> +struct gntdev_dmabuf *
> +gntdev_dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device 
> *dev,
> +   int fd, int count, int domid)
> +{
> + return ERR_PTR(-ENOMEM);
> +}
> +
> +u32 *gntdev_dmabuf_imp_get_refs(struct gntdev_dmabuf *gntdev_dmabuf)
> +{
> + return NULL;
> +}
> +
> +int gntdev_dmabuf_imp_release(struct gntdev_dmabuf_priv *priv, u32 fd)
> +{
> + return -EINVAL;
> +}
> +
> +struct gntdev_dmabuf_priv *gntdev_dmabuf_init(void)
> +{
> + struct 

Re: [PATCH 0/8] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 05/31/2018 10:41 AM, Oleksandr Andrushchenko wrote:
> On 05/31/2018 08:51 AM, Oleksandr Andrushchenko wrote:
>> On 05/31/2018 04:46 AM, Boris Ostrovsky wrote:
>>>
>>>
>>> On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
>>>
>>>>
>>>> Oleksandr Andrushchenko (8):
>>>>    xen/grant-table: Make set/clear page private code shared
>>>>    xen/balloon: Move common memory reservation routines to a module
>>>>    xen/grant-table: Allow allocating buffers suitable for DMA
>>>>    xen/gntdev: Allow mappings for DMA buffers
>>>>    xen/gntdev: Add initial support for dma-buf UAPI
>>>>    xen/gntdev: Implement dma-buf export functionality
>>>>    xen/gntdev: Implement dma-buf import functionality
>>>>    xen/gntdev: Expose gntdev's dma-buf API for in-kernel use
>>>>
>>>>   drivers/xen/Kconfig   |   23 +
>>>>   drivers/xen/Makefile  |    1 +
>>>>   drivers/xen/balloon.c |   71 +--
>>>>   drivers/xen/gntdev.c  | 1025
>>>> -
>>>
>>>
>>> I think this calls for gntdev_dma.c.
>> I assume you mean as a separate file (part of gntdev driver)?
>>> I only had a quick look over gntdev changes but they very much are
>>> concentrated in dma-specific routines.
>>>
>> I tried to do that, but there are some dependencies between the
>> gntdev.c and gntdev_dma.c,
>> so finally I decided to put it all together.
>>> You essentially only share file_operations entry points with
>>> original gntdev code, right?
>>>
>> fops + mappings done by gntdev (struct grant_map) and I need to
>> release map on dma_buf .release
>> callback which makes some cross-dependencies between modules which
>> seemed to be not cute
>> (gntdev keeps its all structs and functions inside, so I cannot
>> easily access those w/o
>> helpers).
>>
>> But I'll try one more time and move all DMA specific stuff into
>> gntdev_dma.c
> Could you please take a quick look at the way I re-structured the
> sources here [1]?
> If this is what you meant.


I looked at final gntdev.c code and I think at least one of the chunks
there ("DMA buffer export support. ") can also be moved out. It still
have a bit too many ifdefs but it looks better to my eye than jamming
everything into a single file (and I think more code can be moved out,
but we can talk about it when you post the patches so that we can see
context).

BTW, I believe it won't build with !CONFIG_XEN_GNTDEV_DMABUF ---
gntdev_remove_map() is defined under this option and is referenced later
without it.


-boris


>
> Thank you,
> Oleksandr
>>> -boris
>>>
>> Thank you,
>> Oleksandr
>>>
>>>>   drivers/xen/grant-table.c |  176 +-
>>>>   drivers/xen/mem-reservation.c |  134 +
>>>>   include/uapi/xen/gntdev.h |  106 
>>>>   include/xen/grant_dev.h   |   37 ++
>>>>   include/xen/grant_table.h |   28 +
>>>>   include/xen/mem_reservation.h |   29 +
>>>>   10 files changed, 1527 insertions(+), 103 deletions(-)
>>>>   create mode 100644 drivers/xen/mem-reservation.c
>>>>   create mode 100644 include/xen/grant_dev.h
>>>>   create mode 100644 include/xen/mem_reservation.h
>>>>
>>
> [1]
> https://github.com/andr2000/linux/commits/xen_tip_linux_next_xen_dma_buf_v2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 05/31/2018 01:51 AM, Oleksandr Andrushchenko wrote:
> On 05/31/2018 04:46 AM, Boris Ostrovsky wrote:
>>
>>
>> On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
>>
>>>
>>> Oleksandr Andrushchenko (8):
>>>    xen/grant-table: Make set/clear page private code shared
>>>    xen/balloon: Move common memory reservation routines to a module
>>>    xen/grant-table: Allow allocating buffers suitable for DMA
>>>    xen/gntdev: Allow mappings for DMA buffers
>>>    xen/gntdev: Add initial support for dma-buf UAPI
>>>    xen/gntdev: Implement dma-buf export functionality
>>>    xen/gntdev: Implement dma-buf import functionality
>>>    xen/gntdev: Expose gntdev's dma-buf API for in-kernel use
>>>
>>>   drivers/xen/Kconfig   |   23 +
>>>   drivers/xen/Makefile  |    1 +
>>>   drivers/xen/balloon.c |   71 +--
>>>   drivers/xen/gntdev.c  | 1025
>>> -
>>
>>
>> I think this calls for gntdev_dma.c.
> I assume you mean as a separate file (part of gntdev driver)?


Yes, source only. The driver stays the same.


>> I only had a quick look over gntdev changes but they very much are
>> concentrated in dma-specific routines.
>>
> I tried to do that, but there are some dependencies between the
> gntdev.c and gntdev_dma.c,
> so finally I decided to put it all together.
>> You essentially only share file_operations entry points with original
>> gntdev code, right?
>>
> fops + mappings done by gntdev (struct grant_map) and I need to
> release map on dma_buf .release
> callback which makes some cross-dependencies between modules which
> seemed to be not cute
> (gntdev keeps its all structs and functions inside, so I cannot easily
> access those w/o
> helpers).
>
> But I'll try one more time and move all DMA specific stuff into
> gntdev_dma.c


Yes, please try it. Maybe even have gntdev_common.c, gntdev_mem.c (??) 
and gntdev_dma.c.

-boris


>> -boris
>>
> Thank you,
> Oleksandr
>>
>>>   drivers/xen/grant-table.c |  176 +-
>>>   drivers/xen/mem-reservation.c |  134 +
>>>   include/uapi/xen/gntdev.h |  106 
>>>   include/xen/grant_dev.h   |   37 ++
>>>   include/xen/grant_table.h |   28 +
>>>   include/xen/mem_reservation.h |   29 +
>>>   10 files changed, 1527 insertions(+), 103 deletions(-)
>>>   create mode 100644 drivers/xen/mem-reservation.c
>>>   create mode 100644 include/xen/grant_dev.h
>>>   create mode 100644 include/xen/mem_reservation.h
>>>
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] xen/balloon: Move common memory reservation routines to a module

[Boris Ostrovsky]

On 05/30/2018 01:46 PM, Oleksandr Andrushchenko wrote:
> On 05/30/2018 06:54 PM, Boris Ostrovsky wrote:
>>
>>
>> BTW, I also think you can further simplify
>> xenmem_reservation_va_mapping_* routines by bailing out right away if
>> xen_feature(XENFEAT_auto_translated_physmap). In fact, you might even
>> make them inlines, along the lines of
>>
>> inline void xenmem_reservation_va_mapping_reset(unsigned long count,
>>  struct page **pages)
>> {
>> #ifdef CONFIG_XEN_HAVE_PVMMU
>> if (!xen_feature(XENFEAT_auto_translated_physmap))
>>     __xenmem_reservation_va_mapping_reset(...)
>> #endif
>> }
> How about:
>
> #ifdef CONFIG_XEN_HAVE_PVMMU
> static inline __xenmem_reservation_va_mapping_reset(struct page *page)
> {
> [...]
> }
> #endif
>
> and
>
> void xenmem_reservation_va_mapping_reset(unsigned long count,
>                      struct page **pages)
> {
> #ifdef CONFIG_XEN_HAVE_PVMMU
>     if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>         int i;
>
>         for (i = 0; i < count; i++)
>             __xenmem_reservation_va_mapping_reset(pages[i]);
>     }
> #endif
> }
>
> This way I can use __xenmem_reservation_va_mapping_reset(page);
> instead of xenmem_reservation_va_mapping_reset(1, );


Sure, this also works.

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] xen/grant-table: Allow allocating buffers suitable for DMA

[Boris Ostrovsky]

On 05/30/2018 02:34 AM, Oleksandr Andrushchenko wrote:
> On 05/29/2018 10:10 PM, Boris Ostrovsky wrote:
>> On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:

>> +/**
>> + * gnttab_dma_free_pages - free DMAable pages
>> + * @args: arguments to the function
>> + */
>> +int gnttab_dma_free_pages(struct gnttab_dma_alloc_args *args)
>> +{
>> +    xen_pfn_t *frames;
>> +    size_t size;
>> +    int i, ret;
>> +
>> +    gnttab_pages_clear_private(args->nr_pages, args->pages);
>> +
>> +    frames = kcalloc(args->nr_pages, sizeof(*frames), GFP_KERNEL);
>>
>> Any way you can do it without allocating memory? One possibility is to
>> keep allocated frames from gnttab_dma_alloc_pages(). (Not sure I like
>> that either but it's the only thing I can think of).
> Yes, I was also thinking about storing the allocated frames array from
> gnttab_dma_alloc_pages(), but that seemed not to be clear enough as
> the caller of the gnttab_dma_alloc_pages will need to store those frames
> in some context, so we can pass them on free. But the caller doesn't
> really
> need the frames which might confuse, so I decided to make those
> allocations
> on the fly.
> But I can still rework that to store the frames if you insist: please
> let me know.


I would prefer not to allocate anything in the release path. Yes, I
realize that dragging frames array around is not necessary but IMO it's
better than potentially failing an allocation during a teardown. A
comment in the struct definition could explain the reason for having
this field.


>>
>>
>>> +    if (!frames)
>>> +    return -ENOMEM;
>>> +
>>> +    for (i = 0; i < args->nr_pages; i++)
>>> +    frames[i] = page_to_xen_pfn(args->pages[i]);
>>
>> Not xen_page_to_gfn()?
> Well, according to [1] it should be :
>     /* XENMEM_populate_physmap requires a PFN based on Xen
>  * granularity.
>  */
>     frame_list[i] = page_to_xen_pfn(page);


Ah, yes. I was looking at decrease_reservation and automatically assumed
the same parameter type.


-boris


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/8] xen: dma-buf support for grant device

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:



Oleksandr Andrushchenko (8):
   xen/grant-table: Make set/clear page private code shared
   xen/balloon: Move common memory reservation routines to a module
   xen/grant-table: Allow allocating buffers suitable for DMA
   xen/gntdev: Allow mappings for DMA buffers
   xen/gntdev: Add initial support for dma-buf UAPI
   xen/gntdev: Implement dma-buf export functionality
   xen/gntdev: Implement dma-buf import functionality
   xen/gntdev: Expose gntdev's dma-buf API for in-kernel use

  drivers/xen/Kconfig   |   23 +
  drivers/xen/Makefile  |1 +
  drivers/xen/balloon.c |   71 +--
  drivers/xen/gntdev.c  | 1025 -



I think this calls for gntdev_dma.c. I only had a quick look over gntdev 
changes but they very much are concentrated in dma-specific routines.


You essentially only share file_operations entry points with original 
gntdev code, right?


-boris



  drivers/xen/grant-table.c |  176 +-
  drivers/xen/mem-reservation.c |  134 +
  include/uapi/xen/gntdev.h |  106 
  include/xen/grant_dev.h   |   37 ++
  include/xen/grant_table.h |   28 +
  include/xen/mem_reservation.h |   29 +
  10 files changed, 1527 insertions(+), 103 deletions(-)
  create mode 100644 drivers/xen/mem-reservation.c
  create mode 100644 include/xen/grant_dev.h
  create mode 100644 include/xen/mem_reservation.h


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] xen/grant-table: Allow allocating buffers suitable for DMA

[Boris Ostrovsky]

On 05/30/2018 01:49 PM, Oleksandr Andrushchenko wrote:
> On 05/30/2018 06:20 PM, Boris Ostrovsky wrote:
>> On 05/30/2018 02:34 AM, Oleksandr Andrushchenko wrote:
>>> On 05/29/2018 10:10 PM, Boris Ostrovsky wrote:
>>>> On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
>>>> +/**
>>>> + * gnttab_dma_free_pages - free DMAable pages
>>>> + * @args: arguments to the function
>>>> + */
>>>> +int gnttab_dma_free_pages(struct gnttab_dma_alloc_args *args)
>>>> +{
>>>> +    xen_pfn_t *frames;
>>>> +    size_t size;
>>>> +    int i, ret;
>>>> +
>>>> +    gnttab_pages_clear_private(args->nr_pages, args->pages);
>>>> +
>>>> +    frames = kcalloc(args->nr_pages, sizeof(*frames), GFP_KERNEL);
>>>>
>>>> Any way you can do it without allocating memory? One possibility is to
>>>> keep allocated frames from gnttab_dma_alloc_pages(). (Not sure I like
>>>> that either but it's the only thing I can think of).
>>> Yes, I was also thinking about storing the allocated frames array from
>>> gnttab_dma_alloc_pages(), but that seemed not to be clear enough as
>>> the caller of the gnttab_dma_alloc_pages will need to store those
>>> frames
>>> in some context, so we can pass them on free. But the caller doesn't
>>> really
>>> need the frames which might confuse, so I decided to make those
>>> allocations
>>> on the fly.
>>> But I can still rework that to store the frames if you insist: please
>>> let me know.
>>
>> I would prefer not to allocate anything in the release path. Yes, I
>> realize that dragging frames array around is not necessary but IMO it's
>> better than potentially failing an allocation during a teardown. A
>> comment in the struct definition could explain the reason for having
>> this field.
> Then I would suggest we have it this way: current API requires that
> struct page **pages are allocated from outside. So, let's allocate
> the frames from outside as well. This way the caller is responsible for
> both pages and frames arrays and API looks consistent.


Yes, that works too.

-boris


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] xen/balloon: Move common memory reservation routines to a module

[Boris Ostrovsky]

On 05/30/2018 04:29 AM, Oleksandr Andrushchenko wrote:
> On 05/29/2018 11:03 PM, Boris Ostrovsky wrote:
>> On 05/29/2018 02:22 PM, Oleksandr Andrushchenko wrote:
>>> On 05/29/2018 09:04 PM, Boris Ostrovsky wrote:
>>>> On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
>>>> @@ -463,11 +457,6 @@ static enum bp_state
>>>> increase_reservation(unsigned long nr_pages)
>>>>    int rc;
>>>>    unsigned long i;
>>>>    struct page   *page;
>>>> -    struct xen_memory_reservation reservation = {
>>>> -    .address_bits = 0,
>>>> -    .extent_order = EXTENT_ORDER,
>>>> -    .domid    = DOMID_SELF
>>>> -    };
>>>>      if (nr_pages > ARRAY_SIZE(frame_list))
>>>>    nr_pages = ARRAY_SIZE(frame_list);
>>>> @@ -486,9 +475,7 @@ static enum bp_state
>>>> increase_reservation(unsigned long nr_pages)
>>>>    page = balloon_next_page(page);
>>>>    }
>>>>    -    set_xen_guest_handle(reservation.extent_start, frame_list);
>>>> -    reservation.nr_extents = nr_pages;
>>>> -    rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, );
>>>> +    rc = xenmem_reservation_increase(nr_pages, frame_list);
>>>>    if (rc <= 0)
>>>>    return BP_EAGAIN;
>>>>    @@ -496,29 +483,7 @@ static enum bp_state
>>>> increase_reservation(unsigned long nr_pages)
>>>>    page = balloon_retrieve(false);
>>>>    BUG_ON(page == NULL);
>>>>    -#ifdef CONFIG_XEN_HAVE_PVMMU
>>>> -    /*
>>>> - * We don't support PV MMU when Linux and Xen is using
>>>> - * different page granularity.
>>>> - */
>>>> -    BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>>>> -
>>>> -    if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>>>> -    unsigned long pfn = page_to_pfn(page);
>>>> -
>>>> -    set_phys_to_machine(pfn, frame_list[i]);
>>>> -
>>>> -    /* Link back into the page tables if not highmem. */
>>>> -    if (!PageHighMem(page)) {
>>>> -    int ret;
>>>> -    ret = HYPERVISOR_update_va_mapping(
>>>> -    (unsigned long)__va(pfn << PAGE_SHIFT),
>>>> -    mfn_pte(frame_list[i], PAGE_KERNEL),
>>>> -    0);
>>>> -    BUG_ON(ret);
>>>> -    }
>>>> -    }
>>>> -#endif
>>>> +    xenmem_reservation_va_mapping_update(1, ,
>>>> _list[i]);
>>>>
>>>> Can you make a single call to xenmem_reservation_va_mapping_update(rc,
>>>> ...)? You need to keep track of pages but presumable they can be put
>>>> into an array (or a list). In fact, perhaps we can have
>>>> balloon_retrieve() return a set of pages.
>>> This is actually how it is used later on for dma-buf, but I just
>>> didn't want
>>> to alter original balloon code too much, but this can be done, in
>>> order of simplicity:
>>>
>>> 1. Similar to frame_list, e.g. static array of struct page* of size
>>> ARRAY_SIZE(frame_list):
>>> more static memory is used, but no allocations
>>>
>>> 2. Allocated at run-time with kcalloc: allocation can fail
>>
>> If this is called in freeing DMA buffer code path or in error path then
>> we shouldn't do it.
>>
>>
>>> 3. Make balloon_retrieve() return a set of pages: will require
>>> list/array allocation
>>> and handling, allocation may fail, balloon_retrieve prototype change
>>
>> balloon pages are strung on the lru list. Can we keep have
>> balloon_retrieve return a list of pages on that list?
> First of all, before we go deep in details, I will highlight
> the goal of the requested change: for balloon driver we call
> xenmem_reservation_va_mapping_update(*1*, , _list[i]);
> from increase_reservation
> and
> xenmem_reservation_va_mapping_reset(*1*, );
> from decrease_reservation and it seems to be not elegant because of
> that one page/frame passed while we might have multiple pages/frames
> passed at once.
>
> In the balloon driver the producer of pages for increase_reservation
> is balloon_retrieve(false) and for decrease_reservation it is
> alloc_page(gfp).
> In case of decrease_reservation the page is added on the list

Re: [PATCH 2/8] xen/balloon: Move common memory reservation routines to a module

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
> +
> +void xenmem_reservation_va_mapping_update(unsigned long count,
> +   struct page **pages,
> +   xen_pfn_t *frames)
> +{
> +#ifdef CONFIG_XEN_HAVE_PVMMU
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + struct page *page;
> +
> + page = pages[i];
> + BUG_ON(page == NULL);
> +
> + /*
> +  * We don't support PV MMU when Linux and Xen is using
> +  * different page granularity.
> +  */
> + BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
> +
> + if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> + unsigned long pfn = page_to_pfn(page);
> +
> + set_phys_to_machine(pfn, frames[i]);
> +
> + /* Link back into the page tables if not highmem. */
> + if (!PageHighMem(page)) {
> + int ret;
> +
> + ret = HYPERVISOR_update_va_mapping(
> + (unsigned long)__va(pfn << 
> PAGE_SHIFT),
> + mfn_pte(frames[i], PAGE_KERNEL),
> + 0);
> + BUG_ON(ret);
> + }
> + }
> + }
> +#endif
> +}
> +EXPORT_SYMBOL(xenmem_reservation_va_mapping_update);
> +
> +void xenmem_reservation_va_mapping_reset(unsigned long count,
> +  struct page **pages)
> +{
> +#ifdef CONFIG_XEN_HAVE_PVMMU
> + int i;
> +
> + for (i = 0; i < count; i++) {
> + /*
> +  * We don't support PV MMU when Linux and Xen is using
> +  * different page granularity.
> +  */
> + BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
> +
> + if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> + struct page *page = pages[i];
> + unsigned long pfn = page_to_pfn(page);
> +
> + if (!PageHighMem(page)) {
> + int ret;
> +
> + ret = HYPERVISOR_update_va_mapping(
> + (unsigned long)__va(pfn << 
> PAGE_SHIFT),
> + __pte_ma(0), 0);
> + BUG_ON(ret);
> + }
> + __set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> + }
> + }
> +#endif
> +}
> +EXPORT_SYMBOL(xenmem_reservation_va_mapping_reset);

One other thing I noticed --- both of these can be declared as NOPs in
the header file if !CONFIG_XEN_HAVE_PVMMU.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] xen/balloon: Move common memory reservation routines to a module

[Boris Ostrovsky]

On 05/29/2018 02:22 PM, Oleksandr Andrushchenko wrote:
> On 05/29/2018 09:04 PM, Boris Ostrovsky wrote:
>> On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:

>> @@ -463,11 +457,6 @@ static enum bp_state
>> increase_reservation(unsigned long nr_pages)
>>   int rc;
>>   unsigned long i;
>>   struct page   *page;
>> -    struct xen_memory_reservation reservation = {
>> -    .address_bits = 0,
>> -    .extent_order = EXTENT_ORDER,
>> -    .domid    = DOMID_SELF
>> -    };
>>     if (nr_pages > ARRAY_SIZE(frame_list))
>>   nr_pages = ARRAY_SIZE(frame_list);
>> @@ -486,9 +475,7 @@ static enum bp_state
>> increase_reservation(unsigned long nr_pages)
>>   page = balloon_next_page(page);
>>   }
>>   -    set_xen_guest_handle(reservation.extent_start, frame_list);
>> -    reservation.nr_extents = nr_pages;
>> -    rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, );
>> +    rc = xenmem_reservation_increase(nr_pages, frame_list);
>>   if (rc <= 0)
>>   return BP_EAGAIN;
>>   @@ -496,29 +483,7 @@ static enum bp_state
>> increase_reservation(unsigned long nr_pages)
>>   page = balloon_retrieve(false);
>>   BUG_ON(page == NULL);
>>   -#ifdef CONFIG_XEN_HAVE_PVMMU
>> -    /*
>> - * We don't support PV MMU when Linux and Xen is using
>> - * different page granularity.
>> - */
>> -    BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>> -
>> -    if (!xen_feature(XENFEAT_auto_translated_physmap)) {
>> -    unsigned long pfn = page_to_pfn(page);
>> -
>> -    set_phys_to_machine(pfn, frame_list[i]);
>> -
>> -    /* Link back into the page tables if not highmem. */
>> -    if (!PageHighMem(page)) {
>> -    int ret;
>> -    ret = HYPERVISOR_update_va_mapping(
>> -    (unsigned long)__va(pfn << PAGE_SHIFT),
>> -    mfn_pte(frame_list[i], PAGE_KERNEL),
>> -    0);
>> -    BUG_ON(ret);
>> -    }
>> -    }
>> -#endif
>> +    xenmem_reservation_va_mapping_update(1, , _list[i]);
>>
>> Can you make a single call to xenmem_reservation_va_mapping_update(rc,
>> ...)? You need to keep track of pages but presumable they can be put
>> into an array (or a list). In fact, perhaps we can have
>> balloon_retrieve() return a set of pages.
> This is actually how it is used later on for dma-buf, but I just
> didn't want
> to alter original balloon code too much, but this can be done, in
> order of simplicity:
>
> 1. Similar to frame_list, e.g. static array of struct page* of size
> ARRAY_SIZE(frame_list):
> more static memory is used, but no allocations
>
> 2. Allocated at run-time with kcalloc: allocation can fail


If this is called in freeing DMA buffer code path or in error path then
we shouldn't do it.


>
> 3. Make balloon_retrieve() return a set of pages: will require
> list/array allocation
> and handling, allocation may fail, balloon_retrieve prototype change


balloon pages are strung on the lru list. Can we keep have
balloon_retrieve return a list of pages on that list?

-boris


>
> Could you please tell which of the above will fit better?
>
>>
>>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] xen/grant-table: Make set/clear page private code shared

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Make set/clear page private code shared and accessible to
> other kernel modules which can re-use these instead of open-coding.
>
> Signed-off-by: Oleksandr Andrushchenko 

Reviewed-by: Boris Ostrovsky 


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] xen/balloon: Move common memory reservation routines to a module

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Memory {increase|decrease}_reservation and VA mappings update/reset
> code used in balloon driver can be made common, so other drivers can
> also re-use the same functionality without open-coding.
> Create a dedicated module 

IIUIC this is not really a module, it's a common file.


> for the shared code and export corresponding
> symbols for other kernel modules.
>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/Makefile  |   1 +
>  drivers/xen/balloon.c |  71 ++
>  drivers/xen/mem-reservation.c | 134 ++
>  include/xen/mem_reservation.h |  29 
>  4 files changed, 170 insertions(+), 65 deletions(-)
>  create mode 100644 drivers/xen/mem-reservation.c
>  create mode 100644 include/xen/mem_reservation.h
>
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 451e833f5931..3c87b0c3aca6 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_HOTPLUG_CPU)+= cpu_hotplug.o
>  obj-$(CONFIG_X86)+= fallback.o
>  obj-y+= grant-table.o features.o balloon.o manage.o preempt.o time.o
> +obj-y+= mem-reservation.o
>  obj-y+= events/
>  obj-y+= xenbus/
>  
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index 065f0b607373..57b482d67a3a 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -71,6 +71,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  static int xen_hotplug_unpopulated;
>  
> @@ -157,13 +158,6 @@ static DECLARE_DELAYED_WORK(balloon_worker, 
> balloon_process);
>  #define GFP_BALLOON \
>   (GFP_HIGHUSER | __GFP_NOWARN | __GFP_NORETRY | __GFP_NOMEMALLOC)
>  
> -static void scrub_page(struct page *page)
> -{
> -#ifdef CONFIG_XEN_SCRUB_PAGES
> - clear_highpage(page);
> -#endif
> -}
> -
>  /* balloon_append: add the given page to the balloon. */
>  static void __balloon_append(struct page *page)
>  {
> @@ -463,11 +457,6 @@ static enum bp_state increase_reservation(unsigned long 
> nr_pages)
>   int rc;
>   unsigned long i;
>   struct page   *page;
> - struct xen_memory_reservation reservation = {
> - .address_bits = 0,
> - .extent_order = EXTENT_ORDER,
> - .domid= DOMID_SELF
> - };
>  
>   if (nr_pages > ARRAY_SIZE(frame_list))
>   nr_pages = ARRAY_SIZE(frame_list);
> @@ -486,9 +475,7 @@ static enum bp_state increase_reservation(unsigned long 
> nr_pages)
>   page = balloon_next_page(page);
>   }
>  
> - set_xen_guest_handle(reservation.extent_start, frame_list);
> - reservation.nr_extents = nr_pages;
> - rc = HYPERVISOR_memory_op(XENMEM_populate_physmap, );
> + rc = xenmem_reservation_increase(nr_pages, frame_list);
>   if (rc <= 0)
>   return BP_EAGAIN;
>  
> @@ -496,29 +483,7 @@ static enum bp_state increase_reservation(unsigned long 
> nr_pages)
>   page = balloon_retrieve(false);
>   BUG_ON(page == NULL);
>  
> -#ifdef CONFIG_XEN_HAVE_PVMMU
> - /*
> -  * We don't support PV MMU when Linux and Xen is using
> -  * different page granularity.
> -  */
> - BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
> -
> - if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> - unsigned long pfn = page_to_pfn(page);
> -
> - set_phys_to_machine(pfn, frame_list[i]);
> -
> - /* Link back into the page tables if not highmem. */
> - if (!PageHighMem(page)) {
> - int ret;
> - ret = HYPERVISOR_update_va_mapping(
> - (unsigned long)__va(pfn << 
> PAGE_SHIFT),
> - mfn_pte(frame_list[i], 
> PAGE_KERNEL),
> - 0);
> - BUG_ON(ret);
> - }
> - }
> -#endif
> + xenmem_reservation_va_mapping_update(1, , _list[i]);


Can you make a single call to xenmem_reservation_va_mapping_update(rc,
...)? You need to keep track of pages but presumable they can be put
into an array (or a list). In fact, perhaps we can have
balloon_retrieve() return a set of pages.




>  
>   /* Relinquish the page back to the allocator. */
>   free_reserved_page(page);
> @@ -535,11 +500,6 @@ static enum bp_state decrease_reservation(unsigned long 
> nr_pages, gfp_t gfp)
>   unsigned long i;
>   struct page *page, *tmp;
>   int ret;
> - struct xen_memory_reservation reservation = {
> - .address_bits = 0,
> - .extent_order = EXTENT_ORDER,
> - .domid= DOMID_SELF
> - };
>   LIST_HEAD(pages);
> 

Re: [PATCH 5/8] xen/gntdev: Add initial support for dma-buf UAPI

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:

>  
> +/*
> + * Create a dma-buf [1] from grant references @refs of count @count provided
> + * by the foreign domain @domid with flags @flags.
> + *
> + * By default dma-buf is backed by system memory pages, but by providing
> + * one of the GNTDEV_DMA_FLAG_XXX flags it can also be created as
> + * a DMA write-combine or coherent buffer, e.g. allocated with dma_alloc_wc/
> + * dma_alloc_coherent.
> + *
> + * Returns 0 if dma-buf was successfully created and the corresponding
> + * dma-buf's file descriptor is returned in @fd.
> + *
> + * [1] 
> https://elixir.bootlin.com/linux/latest/source/Documentation/driver-api/dma-buf.rst


Documentation/driver-api/dma-buf.rst.


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] xen/grant-table: Allow allocating buffers suitable for DMA

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Extend grant table module API to allow allocating buffers that can
> be used for DMA operations and mapping foreign grant references
> on top of those.
> The resulting buffer is similar to the one allocated by the balloon
> driver in terms that proper memory reservation is made
> ({increase|decrease}_reservation and VA mappings updated if needed).
> This is useful for sharing foreign buffers with HW drivers which
> cannot work with scattered buffers provided by the balloon driver,
> but require DMAable memory instead.
>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/Kconfig   |  13 
>  drivers/xen/grant-table.c | 124 ++
>  include/xen/grant_table.h |  25 
>  3 files changed, 162 insertions(+)
>
> diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> index e5d0c28372ea..3431fe210624 100644
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -161,6 +161,19 @@ config XEN_GRANT_DEV_ALLOC
> to other domains. This can be used to implement frontend drivers
> or as part of an inter-domain shared memory channel.
>  
> +config XEN_GRANT_DMA_ALLOC
> + bool "Allow allocating DMA capable buffers with grant reference module"
> + depends on XEN


Should it depend on anything from DMA? CONFIG_HAS_DMA for example?

> + help
> +   Extends grant table module API to allow allocating DMA capable
> +   buffers and mapping foreign grant references on top of it.
> +   The resulting buffer is similar to one allocated by the balloon
> +   driver in terms that proper memory reservation is made
> +   ({increase|decrease}_reservation and VA mappings updated if needed).
> +   This is useful for sharing foreign buffers with HW drivers which
> +   cannot work with scattered buffers provided by the balloon driver,
> +   but require DMAable memory instead.
> +
>  config SWIOTLB_XEN
>   def_bool y
>   select SWIOTLB
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index d7488226e1f2..06fe6e7f639c 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -45,6 +45,9 @@
>  #include 
>  #include 
>  #include 
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> +#include 
> +#endif
>  
>  #include 
>  #include 
> @@ -57,6 +60,7 @@
>  #ifdef CONFIG_X86
>  #include 
>  #endif
> +#include 
>  #include 
>  #include 
>  
> @@ -811,6 +815,82 @@ int gnttab_alloc_pages(int nr_pages, struct page **pages)
>  }
>  EXPORT_SYMBOL(gnttab_alloc_pages);
>  
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> +/**
> + * gnttab_dma_alloc_pages - alloc DMAable pages suitable for grant mapping 
> into
> + * @args: arguments to the function
> + */
> +int gnttab_dma_alloc_pages(struct gnttab_dma_alloc_args *args)
> +{
> + unsigned long pfn, start_pfn;
> + xen_pfn_t *frames;
> + size_t size;
> + int i, ret;
> +
> + frames = kcalloc(args->nr_pages, sizeof(*frames), GFP_KERNEL);
> + if (!frames)
> + return -ENOMEM;
> +
> + size = args->nr_pages << PAGE_SHIFT;
> + if (args->coherent)
> + args->vaddr = dma_alloc_coherent(args->dev, size,
> +  >dev_bus_addr,
> +  GFP_KERNEL | __GFP_NOWARN);
> + else
> + args->vaddr = dma_alloc_wc(args->dev, size,
> +>dev_bus_addr,
> +GFP_KERNEL | __GFP_NOWARN);
> + if (!args->vaddr) {
> + pr_err("Failed to allocate DMA buffer of size %zu\n", size);
> + ret = -ENOMEM;
> + goto fail_free_frames;
> + }
> +
> + start_pfn = __phys_to_pfn(args->dev_bus_addr);
> + for (pfn = start_pfn, i = 0; pfn < start_pfn + args->nr_pages;
> + pfn++, i++) {
> + struct page *page = pfn_to_page(pfn);
> +
> + args->pages[i] = page;
> + frames[i] = xen_page_to_gfn(page);
> + xenmem_reservation_scrub_page(page);
> + }
> +
> + xenmem_reservation_va_mapping_reset(args->nr_pages, args->pages);
> +
> + ret = xenmem_reservation_decrease(args->nr_pages, frames);
> + if (ret != args->nr_pages) {
> + pr_err("Failed to decrease reservation for DMA buffer\n");
> + xenmem_reservation_increase(ret, frames);
> + ret = -EFAULT;
> + goto fail_free_dma;
> + }
> +
> + ret = gnttab_pages_set_private(args->nr_pages, args->pages);
> + if (ret < 0)
> + goto fail_clear_private;
> +
> + kfree(frames);
> + return 0;
> +
> +fail_clear_private:
> + gnttab_pages_clear_private(args->nr_pages, args->pages);
> +fail_free_dma:


Do you need to xenmem_reservation_increase()?

> + xenmem_reservation_va_mapping_update(args->nr_pages, args->pages,
> +

Re: [PATCH 4/8] xen/gntdev: Allow mappings for DMA buffers

[Boris Ostrovsky]

On 05/25/2018 11:33 AM, Oleksandr Andrushchenko wrote:
>  
>  struct unmap_notify {
> @@ -96,10 +104,28 @@ struct grant_map {
>   struct gnttab_unmap_grant_ref *kunmap_ops;
>   struct page **pages;
>   unsigned long pages_vm_start;
> +
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + /*
> +  * If dmabuf_vaddr is not NULL then this mapping is backed by DMA
> +  * capable memory.
> +  */
> +
> + /* Device for which DMA memory is allocated. */
> + struct device *dma_dev;
> + /* Flags used to create this DMA buffer: GNTDEV_DMABUF_FLAG_XXX. */
> + bool dma_flags;

Again, I think most of the comments here can be dropped. Except possibly
for the flags.

> + /* Virtual/CPU address of the DMA buffer. */
> + void *dma_vaddr;
> + /* Bus address of the DMA buffer. */
> + dma_addr_t dma_bus_addr;
> +#endif
>  };
>  
>  static int unmap_grant_pages(struct grant_map *map, int offset, int pages);
>  
> +static struct miscdevice gntdev_miscdev;
> +
>  /* -- */
>  
>  static void gntdev_print_maps(struct gntdev_priv *priv,
> @@ -121,8 +147,26 @@ static void gntdev_free_map(struct grant_map *map)
>   if (map == NULL)
>   return;
>  
> +#ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> + if (map->dma_vaddr) {
> + struct gnttab_dma_alloc_args args;
> +
> + args.dev = map->dma_dev;
> + args.coherent = map->dma_flags & GNTDEV_DMA_FLAG_COHERENT;
> + args.nr_pages = map->count;
> + args.pages = map->pages;
> + args.vaddr = map->dma_vaddr;
> + args.dev_bus_addr = map->dma_bus_addr;
> +
> + gnttab_dma_free_pages();
> + } else if (map->pages) {
> + gnttab_free_pages(map->count, map->pages);
> + }
> +#else
>   if (map->pages)
>   gnttab_free_pages(map->count, map->pages);
> +#endif
> +

} else
#endif
    if (map->pages)
        gnttab_free_pages(map->count, map->pages);


(and elsewhere)

>   kfree(map->pages);
>   kfree(map->grants);
>   kfree(map->map_ops);



>  
> diff --git a/include/uapi/xen/gntdev.h b/include/uapi/xen/gntdev.h
> index 6d1163456c03..2d5a4672f07c 100644
> --- a/include/uapi/xen/gntdev.h
> +++ b/include/uapi/xen/gntdev.h
> @@ -200,4 +200,19 @@ struct ioctl_gntdev_grant_copy {
>  /* Send an interrupt on the indicated event channel */
>  #define UNMAP_NOTIFY_SEND_EVENT 0x2
>  
> +/*
> + * Flags to be used while requesting memory mapping's backing storage
> + * to be allocated with DMA API.
> + */
> +
> +/*
> + * The buffer is backed with memory allocated with dma_alloc_wc.
> + */
> +#define GNTDEV_DMA_FLAG_WC   (1 << 1)


Is there a reason you are not using bit 0?

-boris

> +
> +/*
> + * The buffer is backed with memory allocated with dma_alloc_coherent.
> + */
> +#define GNTDEV_DMA_FLAG_COHERENT (1 << 2)
> +
>  #endif /* __LINUX_PUBLIC_GNTDEV_H__ */

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/22/2018 01:55 AM, Oleksandr Andrushchenko wrote:
> On 05/21/2018 11:36 PM, Boris Ostrovsky wrote:
>> On 05/21/2018 03:13 PM, Oleksandr Andrushchenko wrote:
>>> On 05/21/2018 09:53 PM, Boris Ostrovsky wrote:
>>>> On 05/21/2018 01:32 PM, Oleksandr Andrushchenko wrote:
>>>>> On 05/21/2018 07:35 PM, Boris Ostrovsky wrote:
>>>>>> On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
>>>>>>> On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
>>>>>>>> On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
>>>>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>>>>>> A commit message would be useful.
>>>>>>> Sure, v1 will have it
>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>> <oleksandr_andrushche...@epam.com>
>>>>>>>>>
>>>>>>>>>  for (i = 0; i < nr_pages; i++) {
>>>>>>>>> -    page = alloc_page(gfp);
>>>>>>>>> -    if (page == NULL) {
>>>>>>>>> -    nr_pages = i;
>>>>>>>>> -    state = BP_EAGAIN;
>>>>>>>>> -    break;
>>>>>>>>> +    if (ext_pages) {
>>>>>>>>> +    page = ext_pages[i];
>>>>>>>>> +    } else {
>>>>>>>>> +    page = alloc_page(gfp);
>>>>>>>>> +    if (page == NULL) {
>>>>>>>>> +    nr_pages = i;
>>>>>>>>> +    state = BP_EAGAIN;
>>>>>>>>> +    break;
>>>>>>>>> +    }
>>>>>>>>>  }
>>>>>>>>>  scrub_page(page);
>>>>>>>>>  list_add(>lru, );
>>>>>>>>> @@ -529,7 +565,7 @@ static enum bp_state
>>>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>>>  i = 0;
>>>>>>>>>  list_for_each_entry_safe(page, tmp, , lru) {
>>>>>>>>>  /* XENMEM_decrease_reservation requires a GFN */
>>>>>>>>> -    frame_list[i++] = xen_page_to_gfn(page);
>>>>>>>>> +    frames[i++] = xen_page_to_gfn(page);
>>>>>>>>>    #ifdef CONFIG_XEN_HAVE_PVMMU
>>>>>>>>>  /*
>>>>>>>>> @@ -552,18 +588,22 @@ static enum bp_state
>>>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>>>  #endif
>>>>>>>>>  list_del(>lru);
>>>>>>>>>  -    balloon_append(page);
>>>>>>>>> +    if (!ext_pages)
>>>>>>>>> +    balloon_append(page);
>>>>>>>> So what you are proposing is not really ballooning. You are just
>>>>>>>> piggybacking on existing interfaces, aren't you?
>>>>>>> Sort of. Basically I need to {increase|decrease}_reservation, not
>>>>>>> actually
>>>>>>> allocating ballooned pages.
>>>>>>> Do you think I can simply EXPORT_SYMBOL for
>>>>>>> {increase|decrease}_reservation?
>>>>>>> Any other suggestion?
>>>>>> I am actually wondering how much of that code you end up reusing.
>>>>>> You
>>>>>> pretty much create new code paths in both routines and common code
>>>>>> ends
>>>>>> up being essentially the hypercall.
>>>>> Well, I hoped that it would be easier to maintain if I modify
>>>>> existing
>>>>> code
>>>>> to support both use-cases, but I am also ok to create new routines if
>>>>> this
>>>>> seems to be reasonable - please let me know
>>>>>>     So the question is --- would it make
>>>>>> sense to do all of this separately from the balloon driver?
>>>>> This can be done, but which driver will host this code then? If we
>>>>> move from
>>>>> the balloon driver, then this could go to either gntdev or
>>>>> grant-table.
&g

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/22/2018 02:27 PM, Oleksandr Andrushchenko wrote:
> On 05/22/2018 09:02 PM, Boris Ostrovsky wrote:
>> On 05/22/2018 11:00 AM, Oleksandr Andrushchenko wrote:
>>> On 05/22/2018 05:33 PM, Boris Ostrovsky wrote:
>>>> On 05/22/2018 01:55 AM, Oleksandr Andrushchenko wrote:
>>>>> On 05/21/2018 11:36 PM, Boris Ostrovsky wrote:
>>>>>> On 05/21/2018 03:13 PM, Oleksandr Andrushchenko wrote:
>>>>>>> On 05/21/2018 09:53 PM, Boris Ostrovsky wrote:
>>>>>>>> On 05/21/2018 01:32 PM, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 05/21/2018 07:35 PM, Boris Ostrovsky wrote:
>>>>>>>>>> On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
>>>>>>>>>>>> On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
>>>>>>>>>>>>> From: Oleksandr Andrushchenko
>>>>>>>>>>>>> <oleksandr_andrushche...@epam.com>
>>>>>>>>>>>> A commit message would be useful.
>>>>>>>>>>> Sure, v1 will have it
>>>>>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>>>>>> <oleksandr_andrushche...@epam.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>>    for (i = 0; i < nr_pages; i++) {
>>>>>>>>>>>>> -    page = alloc_page(gfp);
>>>>>>>>>>>>> -    if (page == NULL) {
>>>>>>>>>>>>> -    nr_pages = i;
>>>>>>>>>>>>> -    state = BP_EAGAIN;
>>>>>>>>>>>>> -    break;
>>>>>>>>>>>>> +    if (ext_pages) {
>>>>>>>>>>>>> +    page = ext_pages[i];
>>>>>>>>>>>>> +    } else {
>>>>>>>>>>>>> +    page = alloc_page(gfp);
>>>>>>>>>>>>> +    if (page == NULL) {
>>>>>>>>>>>>> +    nr_pages = i;
>>>>>>>>>>>>> +    state = BP_EAGAIN;
>>>>>>>>>>>>> +    break;
>>>>>>>>>>>>> +    }
>>>>>>>>>>>>>    }
>>>>>>>>>>>>>    scrub_page(page);
>>>>>>>>>>>>>    list_add(>lru, );
>>>>>>>>>>>>> @@ -529,7 +565,7 @@ static enum bp_state
>>>>>>>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>>>>>>>    i = 0;
>>>>>>>>>>>>>    list_for_each_entry_safe(page, tmp, , lru) {
>>>>>>>>>>>>>    /* XENMEM_decrease_reservation requires a
>>>>>>>>>>>>> GFN */
>>>>>>>>>>>>> -    frame_list[i++] = xen_page_to_gfn(page);
>>>>>>>>>>>>> +    frames[i++] = xen_page_to_gfn(page);
>>>>>>>>>>>>>      #ifdef CONFIG_XEN_HAVE_PVMMU
>>>>>>>>>>>>>    /*
>>>>>>>>>>>>> @@ -552,18 +588,22 @@ static enum bp_state
>>>>>>>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>>>>>>>    #endif
>>>>>>>>>>>>>    list_del(>lru);
>>>>>>>>>>>>>    -    balloon_append(page);
>>>>>>>>>>>>> +    if (!ext_pages)
>>>>>>>>>>>>> +    balloon_append(page);
>>>>>>>>>>>> So what you are proposing is not really ballooning. You are
>>>>>>>>>>>> just
>>>>>>>>>>>> piggybacking on existing interfaces, aren't you?
>>>>>>>>>>> Sort of. Basically I need to
>>>>>>>>>>> {increase|decrease}_reservation, not
&g

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/22/2018 11:00 AM, Oleksandr Andrushchenko wrote:
> On 05/22/2018 05:33 PM, Boris Ostrovsky wrote:
>> On 05/22/2018 01:55 AM, Oleksandr Andrushchenko wrote:
>>> On 05/21/2018 11:36 PM, Boris Ostrovsky wrote:
>>>> On 05/21/2018 03:13 PM, Oleksandr Andrushchenko wrote:
>>>>> On 05/21/2018 09:53 PM, Boris Ostrovsky wrote:
>>>>>> On 05/21/2018 01:32 PM, Oleksandr Andrushchenko wrote:
>>>>>>> On 05/21/2018 07:35 PM, Boris Ostrovsky wrote:
>>>>>>>> On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
>>>>>>>>>> On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
>>>>>>>>>>> From: Oleksandr Andrushchenko
>>>>>>>>>>> <oleksandr_andrushche...@epam.com>
>>>>>>>>>> A commit message would be useful.
>>>>>>>>> Sure, v1 will have it
>>>>>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>>>>>> <oleksandr_andrushche...@epam.com>
>>>>>>>>>>>
>>>>>>>>>>>   for (i = 0; i < nr_pages; i++) {
>>>>>>>>>>> -    page = alloc_page(gfp);
>>>>>>>>>>> -    if (page == NULL) {
>>>>>>>>>>> -    nr_pages = i;
>>>>>>>>>>> -    state = BP_EAGAIN;
>>>>>>>>>>> -    break;
>>>>>>>>>>> +    if (ext_pages) {
>>>>>>>>>>> +    page = ext_pages[i];
>>>>>>>>>>> +    } else {
>>>>>>>>>>> +    page = alloc_page(gfp);
>>>>>>>>>>> +    if (page == NULL) {
>>>>>>>>>>> +    nr_pages = i;
>>>>>>>>>>> +    state = BP_EAGAIN;
>>>>>>>>>>> +    break;
>>>>>>>>>>> +    }
>>>>>>>>>>>   }
>>>>>>>>>>>   scrub_page(page);
>>>>>>>>>>>   list_add(>lru, );
>>>>>>>>>>> @@ -529,7 +565,7 @@ static enum bp_state
>>>>>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>>>>>   i = 0;
>>>>>>>>>>>   list_for_each_entry_safe(page, tmp, , lru) {
>>>>>>>>>>>   /* XENMEM_decrease_reservation requires a GFN */
>>>>>>>>>>> -    frame_list[i++] = xen_page_to_gfn(page);
>>>>>>>>>>> +    frames[i++] = xen_page_to_gfn(page);
>>>>>>>>>>>     #ifdef CONFIG_XEN_HAVE_PVMMU
>>>>>>>>>>>   /*
>>>>>>>>>>> @@ -552,18 +588,22 @@ static enum bp_state
>>>>>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>>>>>   #endif
>>>>>>>>>>>   list_del(>lru);
>>>>>>>>>>>   -    balloon_append(page);
>>>>>>>>>>> +    if (!ext_pages)
>>>>>>>>>>> +    balloon_append(page);
>>>>>>>>>> So what you are proposing is not really ballooning. You are just
>>>>>>>>>> piggybacking on existing interfaces, aren't you?
>>>>>>>>> Sort of. Basically I need to {increase|decrease}_reservation, not
>>>>>>>>> actually
>>>>>>>>> allocating ballooned pages.
>>>>>>>>> Do you think I can simply EXPORT_SYMBOL for
>>>>>>>>> {increase|decrease}_reservation?
>>>>>>>>> Any other suggestion?
>>>>>>>> I am actually wondering how much of that code you end up reusing.
>>>>>>>> You
>>>>>>>> pretty much create new code paths in both routines and common code
>>>>>>>> ends
>>>>>>>> up being essentially the hypercall.
>>>>>>> Well, I hoped that it 

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
> On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
>> On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>
>> A commit message would be useful.
> Sure, v1 will have it
>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <oleksandr_andrushche...@epam.com>
>>>
>>>   for (i = 0; i < nr_pages; i++) {
>>> -    page = alloc_page(gfp);
>>> -    if (page == NULL) {
>>> -    nr_pages = i;
>>> -    state = BP_EAGAIN;
>>> -    break;
>>> +    if (ext_pages) {
>>> +    page = ext_pages[i];
>>> +    } else {
>>> +    page = alloc_page(gfp);
>>> +    if (page == NULL) {
>>> +    nr_pages = i;
>>> +    state = BP_EAGAIN;
>>> +    break;
>>> +    }
>>>   }
>>>   scrub_page(page);
>>>   list_add(>lru, );
>>> @@ -529,7 +565,7 @@ static enum bp_state
>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>   i = 0;
>>>   list_for_each_entry_safe(page, tmp, , lru) {
>>>   /* XENMEM_decrease_reservation requires a GFN */
>>> -    frame_list[i++] = xen_page_to_gfn(page);
>>> +    frames[i++] = xen_page_to_gfn(page);
>>>     #ifdef CONFIG_XEN_HAVE_PVMMU
>>>   /*
>>> @@ -552,18 +588,22 @@ static enum bp_state
>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>   #endif
>>>   list_del(>lru);
>>>   -    balloon_append(page);
>>> +    if (!ext_pages)
>>> +    balloon_append(page);
>>
>> So what you are proposing is not really ballooning. You are just
>> piggybacking on existing interfaces, aren't you?
> Sort of. Basically I need to {increase|decrease}_reservation, not
> actually
> allocating ballooned pages.
> Do you think I can simply EXPORT_SYMBOL for
> {increase|decrease}_reservation?
> Any other suggestion?


I am actually wondering how much of that code you end up reusing. You
pretty much create new code paths in both routines and common code ends
up being essentially the hypercall. So the question is --- would it make
sense to do all of this separately from the balloon driver?


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/21/2018 03:13 PM, Oleksandr Andrushchenko wrote:
> On 05/21/2018 09:53 PM, Boris Ostrovsky wrote:
>> On 05/21/2018 01:32 PM, Oleksandr Andrushchenko wrote:
>>> On 05/21/2018 07:35 PM, Boris Ostrovsky wrote:
>>>> On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
>>>>> On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
>>>>>> On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
>>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>>>> A commit message would be useful.
>>>>> Sure, v1 will have it
>>>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>>>> <oleksandr_andrushche...@epam.com>
>>>>>>>
>>>>>>>     for (i = 0; i < nr_pages; i++) {
>>>>>>> -    page = alloc_page(gfp);
>>>>>>> -    if (page == NULL) {
>>>>>>> -    nr_pages = i;
>>>>>>> -    state = BP_EAGAIN;
>>>>>>> -    break;
>>>>>>> +    if (ext_pages) {
>>>>>>> +    page = ext_pages[i];
>>>>>>> +    } else {
>>>>>>> +    page = alloc_page(gfp);
>>>>>>> +    if (page == NULL) {
>>>>>>> +    nr_pages = i;
>>>>>>> +    state = BP_EAGAIN;
>>>>>>> +    break;
>>>>>>> +    }
>>>>>>>     }
>>>>>>>     scrub_page(page);
>>>>>>>     list_add(>lru, );
>>>>>>> @@ -529,7 +565,7 @@ static enum bp_state
>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>     i = 0;
>>>>>>>     list_for_each_entry_safe(page, tmp, , lru) {
>>>>>>>     /* XENMEM_decrease_reservation requires a GFN */
>>>>>>> -    frame_list[i++] = xen_page_to_gfn(page);
>>>>>>> +    frames[i++] = xen_page_to_gfn(page);
>>>>>>>       #ifdef CONFIG_XEN_HAVE_PVMMU
>>>>>>>     /*
>>>>>>> @@ -552,18 +588,22 @@ static enum bp_state
>>>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>>>     #endif
>>>>>>>     list_del(>lru);
>>>>>>>     -    balloon_append(page);
>>>>>>> +    if (!ext_pages)
>>>>>>> +    balloon_append(page);
>>>>>> So what you are proposing is not really ballooning. You are just
>>>>>> piggybacking on existing interfaces, aren't you?
>>>>> Sort of. Basically I need to {increase|decrease}_reservation, not
>>>>> actually
>>>>> allocating ballooned pages.
>>>>> Do you think I can simply EXPORT_SYMBOL for
>>>>> {increase|decrease}_reservation?
>>>>> Any other suggestion?
>>>> I am actually wondering how much of that code you end up reusing. You
>>>> pretty much create new code paths in both routines and common code
>>>> ends
>>>> up being essentially the hypercall.
>>> Well, I hoped that it would be easier to maintain if I modify existing
>>> code
>>> to support both use-cases, but I am also ok to create new routines if
>>> this
>>> seems to be reasonable - please let me know
>>>>    So the question is --- would it make
>>>> sense to do all of this separately from the balloon driver?
>>> This can be done, but which driver will host this code then? If we
>>> move from
>>> the balloon driver, then this could go to either gntdev or grant-table.
>>> What's your preference?
>> A separate module?
>
>> Is there any use for this feature outside of your zero-copy DRM driver?
> Intel's hyper dma-buf (Dongwon/Matt CC'ed), V4L/GPU at least.
>
> At the time I tried to upstream zcopy driver it was discussed and
> decided that
> it would be better if I remove all DRM specific code and move it to
> Xen drivers.
> Thus, this RFC.
>
> But it can also be implemented as a dedicated Xen dma-buf driver which
> will have all the
> code from this RFC + a bit more (char/misc device handling at least).
> This will also require a dedicated user-space library, just like
> libxengnttab.so
> for gntdev (now I have all new IOCTLs covered there).
>
> If the idea of a dedicated Xen dma-buf driver seems to be more
> attractive we
> can work toward this solution. BTW, I do support this idea, but was not
> sure if Xen community accepts yet another driver which duplicates
> quite some code
> of the existing gntdev/balloon/grant-table. And now after this RFC I
> hope that all cons
> and pros of both dedicated driver and gntdev/balloon/grant-table
> extension are
> clearly seen and we can make a decision.


IIRC the objection for a separate module was in the context of gntdev
was discussion, because (among other things) people didn't want to have
yet another file in /dev/xen/

Here we are talking about (a new) balloon-like module which doesn't
create any new user-visible interfaces. And as for duplicating code ---
as I said, I am not convinced there is much of duplication.

I might even argue that we should add a new config option for this module.


-boris

>
>>
>> -boris
> Thank you,
> Oleksandr
> [1]
> https://lists.freedesktop.org/archives/dri-devel/2018-April/173163.html

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/21/2018 01:32 PM, Oleksandr Andrushchenko wrote:
> On 05/21/2018 07:35 PM, Boris Ostrovsky wrote:
>> On 05/21/2018 01:40 AM, Oleksandr Andrushchenko wrote:
>>> On 05/19/2018 01:04 AM, Boris Ostrovsky wrote:
>>>> On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>>>> A commit message would be useful.
>>> Sure, v1 will have it
>>>>> Signed-off-by: Oleksandr Andrushchenko
>>>>> <oleksandr_andrushche...@epam.com>
>>>>>
>>>>>    for (i = 0; i < nr_pages; i++) {
>>>>> -    page = alloc_page(gfp);
>>>>> -    if (page == NULL) {
>>>>> -    nr_pages = i;
>>>>> -    state = BP_EAGAIN;
>>>>> -    break;
>>>>> +    if (ext_pages) {
>>>>> +    page = ext_pages[i];
>>>>> +    } else {
>>>>> +    page = alloc_page(gfp);
>>>>> +    if (page == NULL) {
>>>>> +    nr_pages = i;
>>>>> +    state = BP_EAGAIN;
>>>>> +    break;
>>>>> +    }
>>>>>    }
>>>>>    scrub_page(page);
>>>>>    list_add(>lru, );
>>>>> @@ -529,7 +565,7 @@ static enum bp_state
>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>    i = 0;
>>>>>    list_for_each_entry_safe(page, tmp, , lru) {
>>>>>    /* XENMEM_decrease_reservation requires a GFN */
>>>>> -    frame_list[i++] = xen_page_to_gfn(page);
>>>>> +    frames[i++] = xen_page_to_gfn(page);
>>>>>      #ifdef CONFIG_XEN_HAVE_PVMMU
>>>>>    /*
>>>>> @@ -552,18 +588,22 @@ static enum bp_state
>>>>> decrease_reservation(unsigned long nr_pages, gfp_t gfp)
>>>>>    #endif
>>>>>    list_del(>lru);
>>>>>    -    balloon_append(page);
>>>>> +    if (!ext_pages)
>>>>> +    balloon_append(page);
>>>> So what you are proposing is not really ballooning. You are just
>>>> piggybacking on existing interfaces, aren't you?
>>> Sort of. Basically I need to {increase|decrease}_reservation, not
>>> actually
>>> allocating ballooned pages.
>>> Do you think I can simply EXPORT_SYMBOL for
>>> {increase|decrease}_reservation?
>>> Any other suggestion?
>>
>> I am actually wondering how much of that code you end up reusing. You
>> pretty much create new code paths in both routines and common code ends
>> up being essentially the hypercall.
> Well, I hoped that it would be easier to maintain if I modify existing
> code
> to support both use-cases, but I am also ok to create new routines if
> this
> seems to be reasonable - please let me know
>>   So the question is --- would it make
>> sense to do all of this separately from the balloon driver?
> This can be done, but which driver will host this code then? If we
> move from
> the balloon driver, then this could go to either gntdev or grant-table.
> What's your preference?

A separate module?

Is there any use for this feature outside of your zero-copy DRM driver?

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [RFC 1/3] xen/balloon: Allow allocating DMA buffers

[Boris Ostrovsky]

On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 


A commit message would be useful.


>
> Signed-off-by: Oleksandr Andrushchenko 
>
>   for (i = 0; i < nr_pages; i++) {
> - page = alloc_page(gfp);
> - if (page == NULL) {
> - nr_pages = i;
> - state = BP_EAGAIN;
> - break;
> + if (ext_pages) {
> + page = ext_pages[i];
> + } else {
> + page = alloc_page(gfp);
> + if (page == NULL) {
> + nr_pages = i;
> + state = BP_EAGAIN;
> + break;
> + }
>   }
>   scrub_page(page);
>   list_add(>lru, );
> @@ -529,7 +565,7 @@ static enum bp_state decrease_reservation(unsigned long 
> nr_pages, gfp_t gfp)
>   i = 0;
>   list_for_each_entry_safe(page, tmp, , lru) {
>   /* XENMEM_decrease_reservation requires a GFN */
> - frame_list[i++] = xen_page_to_gfn(page);
> + frames[i++] = xen_page_to_gfn(page);
>  
>  #ifdef CONFIG_XEN_HAVE_PVMMU
>   /*
> @@ -552,18 +588,22 @@ static enum bp_state decrease_reservation(unsigned long 
> nr_pages, gfp_t gfp)
>  #endif
>   list_del(>lru);
>  
> - balloon_append(page);
> + if (!ext_pages)
> + balloon_append(page);


So what you are proposing is not really ballooning. You are just
piggybacking on existing interfaces, aren't you?

-boris


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel][RFC 2/3] xen/grant-table: Extend API to work with DMA buffers

[Boris Ostrovsky]

On 05/17/2018 04:26 AM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Signed-off-by: Oleksandr Andrushchenko 
> ---
>  drivers/xen/grant-table.c | 49 +++
>  include/xen/grant_table.h |  7 ++
>  2 files changed, 56 insertions(+)
>
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index bb36b1e1dbcc..c27bcc420575 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -729,6 +729,55 @@ void gnttab_free_pages(int nr_pages, struct page **pages)
>  }
>  EXPORT_SYMBOL(gnttab_free_pages);
>  
> +int gnttab_dma_alloc_pages(struct device *dev, bool coherent,
> +int nr_pages, struct page **pages,
> +void **vaddr, dma_addr_t *dev_bus_addr)
> +{
> + int i;
> + int ret;
> +
> + ret = alloc_dma_xenballooned_pages(dev, coherent, nr_pages, pages,
> +vaddr, dev_bus_addr);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < nr_pages; i++) {
> +#if BITS_PER_LONG < 64
> + struct xen_page_foreign *foreign;
> +
> + foreign = kzalloc(sizeof(*foreign), GFP_KERNEL);
> + if (!foreign) {
> + gnttab_dma_free_pages(dev, flags, nr_pages, pages,
> +   *vaddr, *dev_bus_addr);
> + return -ENOMEM;
> + }
> + set_page_private(pages[i], (unsigned long)foreign);
> +#endif
> + SetPagePrivate(pages[i]);
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(gnttab_dma_alloc_pages);
> +
> +void gnttab_dma_free_pages(struct device *dev, bool coherent,
> +int nr_pages, struct page **pages,
> +void *vaddr, dma_addr_t dev_bus_addr)
> +{
> + int i;
> +
> + for (i = 0; i < nr_pages; i++) {
> + if (PagePrivate(pages[i])) {
> +#if BITS_PER_LONG < 64
> + kfree((void *)page_private(pages[i]));
> +#endif
> + ClearPagePrivate(pages[i]);
> + }
> + }
> + free_dma_xenballooned_pages(dev, coherent, nr_pages, pages,
> + vaddr, dev_bus_addr);
> +}
> +EXPORT_SYMBOL(gnttab_dma_free_pages);


Given that these routines look almost exactly like their non-dma
counterparts I wonder whether common code could be factored out.

-boris




> +
>  /* Handling of paged out grant targets (GNTST_eagain) */
>  #define MAX_DELAY 256
>  static inline void
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 34b1379f9777..20ee2b5ba965 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -195,6 +195,13 @@ void gnttab_free_auto_xlat_frames(void);
>  int gnttab_alloc_pages(int nr_pages, struct page **pages);
>  void gnttab_free_pages(int nr_pages, struct page **pages);
>  
> +int gnttab_dma_alloc_pages(struct device *dev, bool coherent,
> +int nr_pages, struct page **pages,
> +void **vaddr, dma_addr_t *dev_bus_addr);
> +void gnttab_dma_free_pages(struct device *dev, bool coherent,
> +int nr_pages, struct page **pages,
> +void *vaddr, dma_addr_t dev_bus_addr);
> +
>  int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
>   struct gnttab_map_grant_ref *kmap_ops,
>   struct page **pages, unsigned int count);

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH 0/1] drm/xen-zcopy: Add Xen zero-copy helper DRM driver

[Boris Ostrovsky]

On 04/23/2018 08:10 AM, Oleksandr Andrushchenko wrote:
> On 04/23/2018 02:52 PM, Wei Liu wrote:
>> On Fri, Apr 20, 2018 at 02:25:20PM +0300, Oleksandr Andrushchenko wrote:
>  the gntdev.
>
> I think this is generic enough that it could be implemented by a
> device not tied to Xen. AFAICT the hyper_dma guys also wanted
> something similar to this.
 You can't just wrap random userspace memory into a dma-buf. We've
 just had
 this discussion with kvm/qemu folks, who proposed just that, and
 after a
 bit of discussion they'll now try to have a driver which just wraps a
 memfd into a dma-buf.
>>> So, we have to decide either we introduce a new driver
>>> (say, under drivers/xen/xen-dma-buf) or extend the existing
>>> gntdev/balloon to support dma-buf use-cases.
>>>
>>> Can anybody from Xen community express their preference here?
>>>
>> Oleksandr talked to me on IRC about this, he said a few IOCTLs need to
>> be added to either existing drivers or a new driver.
>>
>> I went through this thread twice and skimmed through the relevant
>> documents, but I couldn't see any obvious pros and cons for either
>> approach. So I don't really have an opinion on this.
>>
>> But, assuming if implemented in existing drivers, those IOCTLs need to
>> be added to different drivers, which means userspace program needs to
>> write more code and get more handles, it would be slightly better to
>> implement a new driver from that perspective.
> If gntdev/balloon extension is still considered:
>
> All the IOCTLs will be in gntdev driver (in current xen-zcopy
> terminology):
>  - DRM_ICOTL_XEN_ZCOPY_DUMB_FROM_REFS
>  - DRM_IOCTL_XEN_ZCOPY_DUMB_TO_REFS
>  - DRM_IOCTL_XEN_ZCOPY_DUMB_WAIT_FREE
>
> Balloon driver extension, which is needed for contiguous/DMA
> buffers, will be to provide new *kernel API*, no UAPI is needed.
>


So I am obviously a bit late to this thread, but why do you need to add
new ioctls to gntdev and balloon? Doesn't this driver manage to do what
you want without any extensions?

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RESEND v2 0/2] drm/xen-front: Add support for Xen PV display frontend

[Boris Ostrovsky]

On 03/13/2018 12:21 PM, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
>
> Hello!
>
> Resending with all the patches squashed on Daniel's request.

Which of the two series are we supposed to review? The 8-patch one or
this? (I hope it's the former)

-boris



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/xen-front: Add support for Xen PV display frontend

[Boris Ostrovsky]

On 03/21/2018 10:58 AM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>

Add support for Xen para-virtualized frontend display driver.
Accompanying backend [1] is implemented as a user-space application
and its helper library [2], capable of running as a Weston client
or DRM master.
Configuration of both backend and frontend is done via
Xen guest domain configuration options [3].



I won't claim that I really understand what's going on here as far as 
DRM stuff is concerned but I didn't see any obvious issues with Xen bits.


So for that you can tack on my
Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/xen-front: Implement GEM operations

[Boris Ostrovsky]

On 02/27/2018 01:52 AM, Oleksandr Andrushchenko wrote:
> On 02/27/2018 01:47 AM, Boris Ostrovsky wrote:
>> On 02/23/2018 10:35 AM, Oleksandr Andrushchenko wrote:
>>> On 02/23/2018 05:26 PM, Boris Ostrovsky wrote:
>>>> On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>>
>>>>> +    ret = gem_alloc_pages_array(xen_obj, size);
>>>>> +    if (ret < 0) {
>>>>> +    gem_free_pages_array(xen_obj);
>>>>> +    goto fail;
>>>>> +    }
>>>>> +
>>>>> +    ret = alloc_xenballooned_pages(xen_obj->num_pages,
>>>>> +    xen_obj->pages);
>>>> Why are you allocating balloon pages?
>>> in this use-case we map pages provided by the backend
>>> (yes, I know this can be a problem from both security
>>> POV and that DomU can die holding pages of Dom0 forever:
>>> but still it is a configuration option, so user decides
>>> if her use-case needs this and takes responsibility for
>>> such a decision).
>>
>> Perhaps I am missing something here but when you say "I know this can be
>> a problem from both security POV ..." then there is something wrong with
>> your solution.
> well, in this scenario there are actually 2 concerns:
> 1. If DomU dies the pages/grants from Dom0/DomD cannot be
> reclaimed back
> 2. Misbehaving guest may send too many requests to the
> backend exhausting grant references and memory of Dom0/DomD
> (this is the only concern from security POV). Please see [1]
>
> But, we are focusing on embedded use-cases,
> so those systems we use are not that "dynamic" with respect to 2).
> Namely: we have fixed number of domains and their functionality
> is well known, so we can do rather precise assumption on resource
> usage. This is why I try to warn on such a use-case and rely on
> the end user who understands the caveats


How will dom0/backend know whether or not to trust the front end (and
thus whether or not to provide provide pages to it)? Will there be
something in xenstore, for example, to indicate such trusted frontends?

-boris


>
> I'll probably add more precise description of this use-case
> clarifying what is that security POV, so there is no confusion
>
> Hope this explanation answers your questions
>> -boris
>>
>>> Please see description of the buffering modes in xen_drm_front.h
>>> specifically for backend allocated buffers:
>>>  
>>> ***
>>>
>>>   * 2. Buffers allocated by the backend
>>>  
>>> ***
>>>
>>>   *
>>>   * This mode of operation is run-time configured via guest domain
>>> configuration
>>>   * through XenStore entries.
>>>   *
>>>   * For systems which do not provide IOMMU support, but having specific
>>>   * requirements for display buffers it is possible to allocate such
>>> buffers
>>>   * at backend side and share those with the frontend.
>>>   * For example, if host domain is 1:1 mapped and has DRM/GPU hardware
>>> expecting
>>>   * physically contiguous memory, this allows implementing zero-copying
>>>   * use-cases.
>>>
>>>> -boris
>>>>
>>>>> +    if (ret < 0) {
>>>>> +    DRM_ERROR("Cannot allocate %zu ballooned pages: %d\n",
>>>>> +    xen_obj->num_pages, ret);
>>>>> +    goto fail;
>>>>> +    }
>>>>> +
>>>>> +    return xen_obj;
>>>>> +    }
>>>>> +    /*
>>>>> + * need to allocate backing pages now, so we can share those
>>>>> + * with the backend
>>>>> + */
>>>>> +    xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
>>>>> +    xen_obj->pages = drm_gem_get_pages(_obj->base);
>>>>> +    if (IS_ERR_OR_NULL(xen_obj->pages)) {
>>>>> +    ret = PTR_ERR(xen_obj->pages);
>>>>> +    xen_obj->pages = NULL;
>>>>> +    goto fail;
>>>>> +    }
>>>>> +
>>>>> +    return xen_obj;
>>>>> +
>>>>> +fail:
>>>>> +    DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
>>>>> +    return ERR_PTR(ret);
>>>>> +}
>>>>> +
>>>>>
> Thank you,
> Oleksandr
>
> [1]
> https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg03100.html
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/xen-front: Implement GEM operations

[Boris Ostrovsky]

On 02/23/2018 10:35 AM, Oleksandr Andrushchenko wrote:
> On 02/23/2018 05:26 PM, Boris Ostrovsky wrote:
>> On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>>> +static struct xen_gem_object *gem_create(struct drm_device *dev,
>>> size_t size)
>>> +{
>>> +struct xen_drm_front_drm_info *drm_info = dev->dev_private;
>>> +struct xen_gem_object *xen_obj;
>>> +int ret;
>>> +
>>> +size = round_up(size, PAGE_SIZE);
>>> +xen_obj = gem_create_obj(dev, size);
>>> +if (IS_ERR_OR_NULL(xen_obj))
>>> +return xen_obj;
>>> +
>>> +if (drm_info->cfg->be_alloc) {
>>> +/*
>>> + * backend will allocate space for this buffer, so
>>> + * only allocate array of pointers to pages
>>> + */
>>> +xen_obj->be_alloc = true;
>> If be_alloc is a flag (which I am not sure about) --- should it be set
>> to true *after* you've successfully allocated your things?
> this is a configuration option telling about the way
> the buffer gets allocated: either by the frontend or
> backend (be_alloc -> buffer allocated by the backend)


I can see how drm_info->cfg->be_alloc might be a configuration option
but xen_obj->be_alloc is set here and that's not how configuration
options typically behave.


>>
>>> +ret = gem_alloc_pages_array(xen_obj, size);
>>> +if (ret < 0) {
>>> +gem_free_pages_array(xen_obj);
>>> +goto fail;
>>> +}
>>> +
>>> +ret = alloc_xenballooned_pages(xen_obj->num_pages,
>>> +xen_obj->pages);
>> Why are you allocating balloon pages?
> in this use-case we map pages provided by the backend
> (yes, I know this can be a problem from both security
> POV and that DomU can die holding pages of Dom0 forever:
> but still it is a configuration option, so user decides
> if her use-case needs this and takes responsibility for
> such a decision).


Perhaps I am missing something here but when you say "I know this can be
a problem from both security POV ..." then there is something wrong with
your solution.

-boris

>
> Please see description of the buffering modes in xen_drm_front.h
> specifically for backend allocated buffers:
>  
> ***
>
>  * 2. Buffers allocated by the backend
>  
> ***
>
>  *
>  * This mode of operation is run-time configured via guest domain
> configuration
>  * through XenStore entries.
>  *
>  * For systems which do not provide IOMMU support, but having specific
>  * requirements for display buffers it is possible to allocate such
> buffers
>  * at backend side and share those with the frontend.
>  * For example, if host domain is 1:1 mapped and has DRM/GPU hardware
> expecting
>  * physically contiguous memory, this allows implementing zero-copying
>  * use-cases.
>
>>
>> -boris
>>
>>> +if (ret < 0) {
>>> +DRM_ERROR("Cannot allocate %zu ballooned pages: %d\n",
>>> +xen_obj->num_pages, ret);
>>> +goto fail;
>>> +}
>>> +
>>> +return xen_obj;
>>> +}
>>> +/*
>>> + * need to allocate backing pages now, so we can share those
>>> + * with the backend
>>> + */
>>> +xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
>>> +xen_obj->pages = drm_gem_get_pages(_obj->base);
>>> +if (IS_ERR_OR_NULL(xen_obj->pages)) {
>>> +ret = PTR_ERR(xen_obj->pages);
>>> +xen_obj->pages = NULL;
>>> +goto fail;
>>> +}
>>> +
>>> +return xen_obj;
>>> +
>>> +fail:
>>> +DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
>>> +return ERR_PTR(ret);
>>> +}
>>> +
>>>
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/xen-front: Implement Xen event channel handling

[Boris Ostrovsky]

On 02/23/2018 02:00 AM, Oleksandr Andrushchenko wrote:
> On 02/23/2018 01:50 AM, Boris Ostrovsky wrote:
>> On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>>> +
>>> +static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
>>> +{
>>> +struct xen_drm_front_evtchnl *evtchnl = dev_id;
>>> +struct xen_drm_front_info *front_info = evtchnl->front_info;
>>> +struct xendispl_resp *resp;
>>> +RING_IDX i, rp;
>>> +unsigned long flags;
>>> +
>>> +spin_lock_irqsave(_info->io_lock, flags);
>>> +
>>> +if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
>>> +goto out;
>> Do you need to check the state under lock? (in other routines too).
> not really, will move out of the lock in interrupt handlers
> other places (I assume you refer to be_stream_do_io)


I was mostly referring to evtchnl_interrupt_evt().

-boris


> it is set under lock as a part of atomic operation, e.g.
> we get a new request pointer from the ring and reset completion
> So, those places still seem to be ok

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/9] drm/xen-front: Introduce DRM/KMS virtual display driver

[Boris Ostrovsky]

On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:

> +
> +struct drm_driver xen_drm_driver = {
> + .driver_features   = DRIVER_GEM | DRIVER_MODESET |
> +  DRIVER_PRIME | DRIVER_ATOMIC,
> + .lastclose = lastclose,
> + .gem_free_object_unlocked  = free_object,
> + .gem_vm_ops= _drm_vm_ops,
> + .prime_handle_to_fd= drm_gem_prime_handle_to_fd,
> + .prime_fd_to_handle= drm_gem_prime_fd_to_handle,
> + .gem_prime_import  = drm_gem_prime_import,
> + .gem_prime_export  = drm_gem_prime_export,
> + .gem_prime_get_sg_table= prime_get_sg_table,
> + .gem_prime_import_sg_table = prime_import_sg_table,
> + .gem_prime_vmap= prime_vmap,
> + .gem_prime_vunmap  = prime_vunmap,
> + .gem_prime_mmap= prime_mmap,
> + .dumb_create   = dumb_create,
> + .fops  = _fops,
> + .name  = "xendrm-du",
> + .desc  = "Xen PV DRM Display Unit",
> + .date  = "20161109",

You must have been working on this for a while ;-)

I assume this needs to be updated.

> +bool xen_drm_front_drv_is_used(struct platform_device *pdev)
> +{
> + struct xen_drm_front_drm_info *drm_info = platform_get_drvdata(pdev);
> + struct drm_device *dev;
> +
> + if (!drm_info)
> + return false;
> +
> + dev = drm_info->drm_dev;
> + if (!dev)
> + return false;
> +
> + /*
> +  * FIXME: the code below must be protected by drm_global_mutex,
> +  * but it is not accessible to us. Anyways there is a race condition,
> +  * but we will re-try.
> +  */
> + return dev->open_count != 0;

Would it be a problem, given the race, if you report that the frontend
is not in use, while it actually is?

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/9] drm/xen-front: Implement handling of shared display buffers

[Boris Ostrovsky]

On 02/23/2018 02:53 AM, Oleksandr Andrushchenko wrote:
> On 02/23/2018 02:25 AM, Boris Ostrovsky wrote:
>> On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>>> static int __init xen_drv_init(void)
>>>   {
>>> +/* At the moment we only support case with XEN_PAGE_SIZE ==
>>> PAGE_SIZE */
>>> +BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);
>>
>> Why BUILD_BUG_ON? This should simply not load if page sizes are
>> different.
>>
>>   
> This is a compile time check, so if kernel/Xen is configured
> to use page size combination which is not supported by the
> driver it will fail during compilation. This seems correct to me,
> because you shouldn't even try to load the driver which
> cannot handle different page sizes to not make any harm.


This will prevent whole kernel from building. So, for example,
randconfig builds will fail.


>>
>>
>>
>>> +ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
>>> +BUG_ON(ret);
>>
>> We should try not to BUG*(). There are a few in this patch (and possibly
>> others) that I think can be avoided.
>>
> I will rework BUG_* for map/unmap code to handle errors,
> but will still leave
> /* either pages or sgt, not both */
> BUG_ON(cfg->pages && cfg->sgt);
> which is a real driver bug and must not happen

Why not return an error?

In fact, AFAICS you only call it in patch 9 where both of these can be
tested, in which case something like -EINVAL would look reasonable.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/xen-front: Introduce Xen para-virtualized frontend driver

[Boris Ostrovsky]

On 02/23/2018 01:37 AM, Oleksandr Andrushchenko wrote:
> On 02/23/2018 12:23 AM, Boris Ostrovsky wrote:
>> On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>>> +static struct xenbus_driver xen_driver = {
>>> +.ids = xen_drv_ids,
>>> +.probe = xen_drv_probe,
>>> +.remove = xen_drv_remove,
>>> +.otherend_changed = backend_on_changed,
>> What does "_on_" stand for?
> Well, it is somewhat like a hint that this is called "on" event,
> e.g. event when the other end state has changed, backend in this
> case. It could be something like "backend_on_state_changed"

If you look at other xenbus_drivers none of the uses this so I think we
should stick to conventional naming. (and the same applies to other
backend_on_* routines).

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/xen-front: Implement GEM operations

[Boris Ostrovsky]

On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
> +static struct xen_gem_object *gem_create(struct drm_device *dev, size_t size)
> +{
> + struct xen_drm_front_drm_info *drm_info = dev->dev_private;
> + struct xen_gem_object *xen_obj;
> + int ret;
> +
> + size = round_up(size, PAGE_SIZE);
> + xen_obj = gem_create_obj(dev, size);
> + if (IS_ERR_OR_NULL(xen_obj))
> + return xen_obj;
> +
> + if (drm_info->cfg->be_alloc) {
> + /*
> +  * backend will allocate space for this buffer, so
> +  * only allocate array of pointers to pages
> +  */
> + xen_obj->be_alloc = true;

If be_alloc is a flag (which I am not sure about) --- should it be set
to true *after* you've successfully allocated your things?

> + ret = gem_alloc_pages_array(xen_obj, size);
> + if (ret < 0) {
> + gem_free_pages_array(xen_obj);
> + goto fail;
> + }
> +
> + ret = alloc_xenballooned_pages(xen_obj->num_pages,
> + xen_obj->pages);

Why are you allocating balloon pages?

-boris

> + if (ret < 0) {
> + DRM_ERROR("Cannot allocate %zu ballooned pages: %d\n",
> + xen_obj->num_pages, ret);
> + goto fail;
> + }
> +
> + return xen_obj;
> + }
> + /*
> +  * need to allocate backing pages now, so we can share those
> +  * with the backend
> +  */
> + xen_obj->num_pages = DIV_ROUND_UP(size, PAGE_SIZE);
> + xen_obj->pages = drm_gem_get_pages(_obj->base);
> + if (IS_ERR_OR_NULL(xen_obj->pages)) {
> + ret = PTR_ERR(xen_obj->pages);
> + xen_obj->pages = NULL;
> + goto fail;
> + }
> +
> + return xen_obj;
> +
> +fail:
> + DRM_ERROR("Failed to allocate buffer with size %zu\n", size);
> + return ERR_PTR(ret);
> +}
> +
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm/xen-front: Introduce Xen para-virtualized frontend driver

[Boris Ostrovsky]

On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
> +static struct xenbus_driver xen_driver = {
> + .ids = xen_drv_ids,
> + .probe = xen_drv_probe,
> + .remove = xen_drv_remove,
> + .otherend_changed = backend_on_changed,

What does "_on_" stand for?

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/9] drm/xen-front: Implement Xen event channel handling

[Boris Ostrovsky]

On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
> +
> +static irqreturn_t evtchnl_interrupt_ctrl(int irq, void *dev_id)
> +{
> + struct xen_drm_front_evtchnl *evtchnl = dev_id;
> + struct xen_drm_front_info *front_info = evtchnl->front_info;
> + struct xendispl_resp *resp;
> + RING_IDX i, rp;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_info->io_lock, flags);
> +
> + if (unlikely(evtchnl->state != EVTCHNL_STATE_CONNECTED))
> + goto out;

Do you need to check the state under lock? (in other routines too).

...

> +
> +static void evtchnl_free(struct xen_drm_front_info *front_info,
> + struct xen_drm_front_evtchnl *evtchnl)
> +{
> + unsigned long page = 0;
> +
> + if (evtchnl->type == EVTCHNL_TYPE_REQ)
> + page = (unsigned long)evtchnl->u.req.ring.sring;
> + else if (evtchnl->type == EVTCHNL_TYPE_EVT)
> + page = (unsigned long)evtchnl->u.evt.page;
> + if (!page)
> + return;
> +
> + evtchnl->state = EVTCHNL_STATE_DISCONNECTED;
> +
> + if (evtchnl->type == EVTCHNL_TYPE_REQ) {
> + /* release all who still waits for response if any */
> + evtchnl->u.req.resp_status = -EIO;
> + complete_all(>u.req.completion);
> + }
> +
> + if (evtchnl->irq)
> + unbind_from_irqhandler(evtchnl->irq, evtchnl);
> +
> + if (evtchnl->port)
> + xenbus_free_evtchn(front_info->xb_dev, evtchnl->port);
> +
> + /* end access and free the page */
> + if (evtchnl->gref != GRANT_INVALID_REF)
> + gnttab_end_foreign_access(evtchnl->gref, 0, page);
> +
> + if (evtchnl->type == EVTCHNL_TYPE_REQ)
> + evtchnl->u.req.ring.sring = NULL;
> + else
> + evtchnl->u.evt.page = NULL;
> +
> + memset(evtchnl, 0, sizeof(*evtchnl));

Since you are zeroing out the structure you don't need to set fields to
zero.

I also think you need to free the page.

-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/9] drm/xen-front: Read driver configuration from Xen store

[Boris Ostrovsky]

On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
> +
> +static int cfg_connector(struct xen_drm_front_info *front_info,
> + struct xen_drm_front_cfg_connector *connector,
> + const char *path, int index)
> +{
> + char *connector_path;
> +
> + connector_path = devm_kasprintf(_info->xb_dev->dev,
> + GFP_KERNEL, "%s/%d", path, index);
> + if (!connector_path)
> + return -ENOMEM;
> +
> + connector->xenstore_path = connector_path;
> + if (xenbus_scanf(XBT_NIL, connector_path, XENDISPL_FIELD_RESOLUTION,
> + "%d" XENDISPL_RESOLUTION_SEPARATOR "%d",
> + >width, >height) < 0) {
> + /* either no entry configured or wrong resolution set */
> + connector->width = 0;
> + connector->height = 0;

Do you also need to set connector->xenstore_path to NULL? Or maybe just
set it after xenbus_scanf() call.

-boris



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/9] drm/xen-front: Implement handling of shared display buffers

[Boris Ostrovsky]

On 02/21/2018 03:03 AM, Oleksandr Andrushchenko wrote:
>  
>  static int __init xen_drv_init(void)
>  {
> + /* At the moment we only support case with XEN_PAGE_SIZE == PAGE_SIZE */
> + BUILD_BUG_ON(XEN_PAGE_SIZE != PAGE_SIZE);


Why BUILD_BUG_ON? This should simply not load if page sizes are different.

 



> + ret = gnttab_map_refs(map_ops, NULL, buf->pages, buf->num_pages);
> + BUG_ON(ret);


We should try not to BUG*(). There are a few in this patch (and possibly
others) that I think can be avoided.





> +
> +static int alloc_storage(struct xen_drm_front_shbuf *buf)
> +{
> + if (buf->sgt) {
> + buf->pages = kvmalloc_array(buf->num_pages,
> + sizeof(struct page *), GFP_KERNEL);
> + if (!buf->pages)
> + return -ENOMEM;
> +
> + if (drm_prime_sg_to_page_addr_arrays(buf->sgt, buf->pages,
> + NULL, buf->num_pages) < 0)
> + return -EINVAL;
> + }
> +
> + buf->grefs = kcalloc(buf->num_grefs, sizeof(*buf->grefs), GFP_KERNEL);
> + if (!buf->grefs)
> + return -ENOMEM;
> +
> + buf->directory = kcalloc(get_num_pages_dir(buf), PAGE_SIZE, GFP_KERNEL);
> + if (!buf->directory)
> + return -ENOMEM;

You need to clean up on errors.

-boris

> +
> + return 0;
> +}

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/28/2017 04:12 AM, Christian König wrote:
>
>
> How about the attached patch? It limits the newly added MMIO space to
> the upper 256GB of the address space. That should still be enough for
> most devices, but we avoid both issues with Xen dom0 as most likely
> problems with memory hotplug as well.

It certainly makes the problem to be less likely so I guess we could do
this for now.

Thanks.
-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/23/2017 09:12 AM, Boris Ostrovsky wrote:
>
>
> On 11/23/2017 03:11 AM, Christian König wrote:
>> Am 22.11.2017 um 18:27 schrieb Boris Ostrovsky:
>>> On 11/22/2017 11:54 AM, Christian König wrote:
>>>> Am 22.11.2017 um 17:24 schrieb Boris Ostrovsky:
>>>>> On 11/22/2017 05:09 AM, Christian König wrote:
>>>>>> Am 21.11.2017 um 23:26 schrieb Boris Ostrovsky:
>>>>>>> On 11/21/2017 08:34 AM, Christian König wrote:
>>>>>>>> Hi Boris,
>>>>>>>>
>>>>>>>> attached are two patches.
>>>>>>>>
>>>>>>>> The first one is a trivial fix for the infinite loop issue, it now
>>>>>>>> correctly aborts the fixup when it can't find address space for
>>>>>>>> the
>>>>>>>> root window.
>>>>>>>>
>>>>>>>> The second is a workaround for your board. It simply checks if
>>>>>>>> there
>>>>>>>> is exactly one Processor Function to apply this fix on.
>>>>>>>>
>>>>>>>> Both are based on linus current master branch. Please test if they
>>>>>>>> fix
>>>>>>>> your issue.
>>>>>>> Yes, they do fix it but that's because the feature is disabled.
>>>>>>>
>>>>>>> Do you know what the actual problem was (on Xen)?
>>>>>> I still haven't understood what you actually did with Xen.
>>>>>>
>>>>>> When you used PCI pass through with those devices then you have
>>>>>> made a
>>>>>> major configuration error.
>>>>>>
>>>>>> When the problem happened on dom0 then the explanation is most
>>>>>> likely
>>>>>> that some PCI device ended up in the configured space, but the
>>>>>> routing
>>>>>> was only setup correctly on one CPU socket.
>>>>> The problem is that dom0 can be (and was in my case() booted with
>>>>> less
>>>>> than full physical memory and so the "rest" of the host memory is not
>>>>> necessarily reflected in iomem. Your patch then tried to configure
>>>>> that
>>>>> memory for MMIO and the system hang.
>>>>>
>>>>> And so my guess is that this patch will break dom0 on a single-socket
>>>>> system as well.
>>>> Oh, thanks!
>>>>
>>>> I've thought about that possibility before, but wasn't able to find a
>>>> system which actually does that.
>>>>
>>>> May I ask why the rest of the memory isn't reported to the OS?
>>> That memory doesn't belong to the OS (dom0), it is owned by the
>>> hypervisor.
>>>
>>>> Sounds like I can't trust Linux resource management and probably need
>>>> to read the DRAM config to figure things out after all.
>>>
>>> My question is whether what you are trying to do should ever be done
>>> for
>>> a guest at all (any guest, not necessarily Xen).
>>
>> The issue is probably that I don't know enough about Xen: What
>> exactly is dom0? My understanding was that dom0 is the hypervisor,
>> but that seems to be incorrect.
>>
>> The issue is that under no circumstances *EVER* a virtualized guest
>> should have access to the PCI devices marked as "Processor Function"
>> on AMD platforms. Otherwise it is trivial to break out of the
>> virtualization.
>>
>> When dom0 is something like the system domain with all hardware
>> access then the approach seems legitimate, but then the hypervisor
>> should report the stolen memory to the OS using the e820 table.
>>
>> When the hypervisor doesn't do that and the Linux kernel isn't aware
>> that there is memory at a given location mapping PCI space there will
>> obviously crash the hypervisor.
>>
>> Possible solutions as far as I can see are either disabling this
>> feature when we detect that we are a Xen dom0, scanning the DRAM
>> settings to update Linux resource handling or fixing Xen to report
>> stolen memory to the dom0 OS as reserved.
>>
>> Opinions?
>
> You are right, these functions are not exposed to a regular guest.
>
> I think for dom0 (which is a special Xen guest, with additional
> privileges) we may be able to add a reserved e820 region for host
> memory that is not assigned to dom0. Let me try it on Monday (I am out
> until then).


One thing I realized while looking at solution for Xen dom0 is that this
patch may cause problems for memory hotplug. What happens if new memory
is added to the system and we have everything above current memory set
to MMIO?

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/23/2017 03:11 AM, Christian König wrote:

Am 22.11.2017 um 18:27 schrieb Boris Ostrovsky:

On 11/22/2017 11:54 AM, Christian König wrote:

Am 22.11.2017 um 17:24 schrieb Boris Ostrovsky:

On 11/22/2017 05:09 AM, Christian König wrote:

Am 21.11.2017 um 23:26 schrieb Boris Ostrovsky:

On 11/21/2017 08:34 AM, Christian König wrote:

Hi Boris,

attached are two patches.

The first one is a trivial fix for the infinite loop issue, it now
correctly aborts the fixup when it can't find address space for the
root window.

The second is a workaround for your board. It simply checks if there
is exactly one Processor Function to apply this fix on.

Both are based on linus current master branch. Please test if they
fix
your issue.

Yes, they do fix it but that's because the feature is disabled.

Do you know what the actual problem was (on Xen)?

I still haven't understood what you actually did with Xen.

When you used PCI pass through with those devices then you have made a
major configuration error.

When the problem happened on dom0 then the explanation is most likely
that some PCI device ended up in the configured space, but the routing
was only setup correctly on one CPU socket.

The problem is that dom0 can be (and was in my case() booted with less
than full physical memory and so the "rest" of the host memory is not
necessarily reflected in iomem. Your patch then tried to configure that
memory for MMIO and the system hang.

And so my guess is that this patch will break dom0 on a single-socket
system as well.

Oh, thanks!

I've thought about that possibility before, but wasn't able to find a
system which actually does that.

May I ask why the rest of the memory isn't reported to the OS?
That memory doesn't belong to the OS (dom0), it is owned by the 
hypervisor.



Sounds like I can't trust Linux resource management and probably need
to read the DRAM config to figure things out after all.


My question is whether what you are trying to do should ever be done for
a guest at all (any guest, not necessarily Xen).


The issue is probably that I don't know enough about Xen: What exactly 
is dom0? My understanding was that dom0 is the hypervisor, but that 
seems to be incorrect.


The issue is that under no circumstances *EVER* a virtualized guest 
should have access to the PCI devices marked as "Processor Function" on 
AMD platforms. Otherwise it is trivial to break out of the virtualization.


When dom0 is something like the system domain with all hardware access 
then the approach seems legitimate, but then the hypervisor should 
report the stolen memory to the OS using the e820 table.


When the hypervisor doesn't do that and the Linux kernel isn't aware 
that there is memory at a given location mapping PCI space there will 
obviously crash the hypervisor.


Possible solutions as far as I can see are either disabling this feature 
when we detect that we are a Xen dom0, scanning the DRAM settings to 
update Linux resource handling or fixing Xen to report stolen memory to 
the dom0 OS as reserved.


Opinions?


You are right, these functions are not exposed to a regular guest.

I think for dom0 (which is a special Xen guest, with additional 
privileges) we may be able to add a reserved e820 region for host memory 
that is not assigned to dom0. Let me try it on Monday (I am out until then).


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/22/2017 05:09 AM, Christian König wrote:
> Am 21.11.2017 um 23:26 schrieb Boris Ostrovsky:
>> On 11/21/2017 08:34 AM, Christian König wrote:
>>> Hi Boris,
>>>
>>> attached are two patches.
>>>
>>> The first one is a trivial fix for the infinite loop issue, it now
>>> correctly aborts the fixup when it can't find address space for the
>>> root window.
>>>
>>> The second is a workaround for your board. It simply checks if there
>>> is exactly one Processor Function to apply this fix on.
>>>
>>> Both are based on linus current master branch. Please test if they fix
>>> your issue.
>>
>> Yes, they do fix it but that's because the feature is disabled.
>>
>> Do you know what the actual problem was (on Xen)?
>
> I still haven't understood what you actually did with Xen.
>
> When you used PCI pass through with those devices then you have made a
> major configuration error.
>
> When the problem happened on dom0 then the explanation is most likely
> that some PCI device ended up in the configured space, but the routing
> was only setup correctly on one CPU socket.

The problem is that dom0 can be (and was in my case() booted with less
than full physical memory and so the "rest" of the host memory is not
necessarily reflected in iomem. Your patch then tried to configure that
memory for MMIO and the system hang.

And so my guess is that this patch will break dom0 on a single-socket
system as well.

-boris

>
> Regards,
> Christian.
>
>>
>> Thanks.
>> -boris
>>
>>> Thanks for the help,
>>> Christian.
>>>
>>> Am 20.11.2017 um 17:33 schrieb Boris Ostrovsky:
>>>> On 11/20/2017 11:07 AM, Christian König wrote:
>>>>> Am 20.11.2017 um 16:51 schrieb Boris Ostrovsky:
>>>>>> (and then it breaks differently as a Xen guest --- we hung on the
>>>>>> last
>>>>>> pci_read_config_dword(), I haven't looked at this at all yet)
>>>>> Hui? How does this fix applies to a Xen guest in the first place?
>>>>>
>>>>> Please provide the output of "lspci -nn" and explain further what is
>>>>> your config with Xen.
>>>>>
>>>>>
>>>> This is dom0.
>>>>
>>>> -bash-4.1# lspci -nn
>>>> 00:00.0 Host bridge [0600]: ATI Technologies Inc RD890 Northbridge
>>>> only
>>>> dual slot (2x16) PCI-e GFX Hydra part [1002:5a10] (rev 02)
>>>> 00:00.2 Generic system peripheral [0806]: ATI Technologies Inc Device
>>>> [1002:5a23]
>>>> 00:0d.0 PCI bridge [0604]: ATI Technologies Inc RD890 PCI to PCI
>>>> bridge
>>>> (external gfx1 port B) [1002:5a1e]
>>>> 00:11.0 SATA controller [0106]: ATI Technologies Inc SB700/SB800 SATA
>>>> Controller [AHCI mode] [1002:4391]
>>>> 00:12.0 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>>>> OHCI0 Controller [1002:4397]
>>>> 00:12.1 USB Controller [0c03]: ATI Technologies Inc SB700 USB OHCI1
>>>> Controller [1002:4398]
>>>> 00:12.2 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>>>> EHCI
>>>> Controller [1002:4396]
>>>> 00:13.0 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>>>> OHCI0 Controller [1002:4397]
>>>> 00:13.1 USB Controller [0c03]: ATI Technologies Inc SB700 USB OHCI1
>>>> Controller [1002:4398]
>>>> 00:13.2 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>>>> EHCI
>>>> Controller [1002:4396]
>>>> 00:14.0 SMBus [0c05]: ATI Technologies Inc SBx00 SMBus Controller
>>>> [1002:4385] (rev 3d)
>>>> 00:14.3 ISA bridge [0601]: ATI Technologies Inc SB700/SB800 LPC host
>>>> controller [1002:439d]
>>>> 00:14.4 PCI bridge [0604]: ATI Technologies Inc SBx00 PCI to PCI
>>>> Bridge
>>>> [1002:4384]
>>>> 00:14.5 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>>>> OHCI2 Controller [1002:4399]
>>>> 00:18.0 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>>>> [1022:1600]
>>>> 00:18.1 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>>>> [1022:1601]
>>>> 00:18.2 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>>>> [1022:1602]
>>>> 00:18.3 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>>>> [1022:1603]
>>>> 00:18.4 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/22/2017 11:54 AM, Christian König wrote:
> Am 22.11.2017 um 17:24 schrieb Boris Ostrovsky:
>> On 11/22/2017 05:09 AM, Christian König wrote:
>>> Am 21.11.2017 um 23:26 schrieb Boris Ostrovsky:
>>>> On 11/21/2017 08:34 AM, Christian König wrote:
>>>>> Hi Boris,
>>>>>
>>>>> attached are two patches.
>>>>>
>>>>> The first one is a trivial fix for the infinite loop issue, it now
>>>>> correctly aborts the fixup when it can't find address space for the
>>>>> root window.
>>>>>
>>>>> The second is a workaround for your board. It simply checks if there
>>>>> is exactly one Processor Function to apply this fix on.
>>>>>
>>>>> Both are based on linus current master branch. Please test if they
>>>>> fix
>>>>> your issue.
>>>> Yes, they do fix it but that's because the feature is disabled.
>>>>
>>>> Do you know what the actual problem was (on Xen)?
>>> I still haven't understood what you actually did with Xen.
>>>
>>> When you used PCI pass through with those devices then you have made a
>>> major configuration error.
>>>
>>> When the problem happened on dom0 then the explanation is most likely
>>> that some PCI device ended up in the configured space, but the routing
>>> was only setup correctly on one CPU socket.
>> The problem is that dom0 can be (and was in my case() booted with less
>> than full physical memory and so the "rest" of the host memory is not
>> necessarily reflected in iomem. Your patch then tried to configure that
>> memory for MMIO and the system hang.
>>
>> And so my guess is that this patch will break dom0 on a single-socket
>> system as well.
>
> Oh, thanks!
>
> I've thought about that possibility before, but wasn't able to find a
> system which actually does that.
>
> May I ask why the rest of the memory isn't reported to the OS?

That memory doesn't belong to the OS (dom0), it is owned by the hypervisor.

>
> Sounds like I can't trust Linux resource management and probably need
> to read the DRAM config to figure things out after all.


My question is whether what you are trying to do should ever be done for
a guest at all (any guest, not necessarily Xen).

-boris

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/21/2017 08:34 AM, Christian König wrote:
> Hi Boris,
>
> attached are two patches.
>
> The first one is a trivial fix for the infinite loop issue, it now
> correctly aborts the fixup when it can't find address space for the
> root window.
>
> The second is a workaround for your board. It simply checks if there
> is exactly one Processor Function to apply this fix on.
>
> Both are based on linus current master branch. Please test if they fix
> your issue.


Yes, they do fix it but that's because the feature is disabled.

Do you know what the actual problem was (on Xen)?

Thanks.
-boris

>
> Thanks for the help,
> Christian.
>
> Am 20.11.2017 um 17:33 schrieb Boris Ostrovsky:
>> On 11/20/2017 11:07 AM, Christian König wrote:
>>> Am 20.11.2017 um 16:51 schrieb Boris Ostrovsky:
>>>> (and then it breaks differently as a Xen guest --- we hung on the last
>>>> pci_read_config_dword(), I haven't looked at this at all yet)
>>> Hui? How does this fix applies to a Xen guest in the first place?
>>>
>>> Please provide the output of "lspci -nn" and explain further what is
>>> your config with Xen.
>>>
>>>
>>
>> This is dom0.
>>
>> -bash-4.1# lspci -nn
>> 00:00.0 Host bridge [0600]: ATI Technologies Inc RD890 Northbridge only
>> dual slot (2x16) PCI-e GFX Hydra part [1002:5a10] (rev 02)
>> 00:00.2 Generic system peripheral [0806]: ATI Technologies Inc Device
>> [1002:5a23]
>> 00:0d.0 PCI bridge [0604]: ATI Technologies Inc RD890 PCI to PCI bridge
>> (external gfx1 port B) [1002:5a1e]
>> 00:11.0 SATA controller [0106]: ATI Technologies Inc SB700/SB800 SATA
>> Controller [AHCI mode] [1002:4391]
>> 00:12.0 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>> OHCI0 Controller [1002:4397]
>> 00:12.1 USB Controller [0c03]: ATI Technologies Inc SB700 USB OHCI1
>> Controller [1002:4398]
>> 00:12.2 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB EHCI
>> Controller [1002:4396]
>> 00:13.0 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>> OHCI0 Controller [1002:4397]
>> 00:13.1 USB Controller [0c03]: ATI Technologies Inc SB700 USB OHCI1
>> Controller [1002:4398]
>> 00:13.2 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB EHCI
>> Controller [1002:4396]
>> 00:14.0 SMBus [0c05]: ATI Technologies Inc SBx00 SMBus Controller
>> [1002:4385] (rev 3d)
>> 00:14.3 ISA bridge [0601]: ATI Technologies Inc SB700/SB800 LPC host
>> controller [1002:439d]
>> 00:14.4 PCI bridge [0604]: ATI Technologies Inc SBx00 PCI to PCI Bridge
>> [1002:4384]
>> 00:14.5 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
>> OHCI2 Controller [1002:4399]
>> 00:18.0 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1600]
>> 00:18.1 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1601]
>> 00:18.2 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1602]
>> 00:18.3 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1603]
>> 00:18.4 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1604]
>> 00:18.5 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1605]
>> 00:19.0 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1600]
>> 00:19.1 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1601]
>> 00:19.2 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1602]
>> 00:19.3 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1603]
>> 00:19.4 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1604]
>> 00:19.5 Host bridge [0600]: Advanced Micro Devices [AMD] Device
>> [1022:1605]
>> 01:04.0 VGA compatible controller [0300]: Matrox Graphics, Inc. MGA
>> G200eW WPCM450 [102b:0532] (rev 0a)
>> 02:00.0 Ethernet controller [0200]: Intel Corporation 82576 Gigabit
>> Network Connection [8086:10c9] (rev 01)
>> 02:00.1 Ethernet controller [0200]: Intel Corporation 82576 Gigabit
>> Network Connection [8086:10c9] (rev 01)
>> -bash-4.1#
>>
>>
>> -boris
>
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 11/20/2017 11:07 AM, Christian König wrote:
> Am 20.11.2017 um 16:51 schrieb Boris Ostrovsky:
>>
>> (and then it breaks differently as a Xen guest --- we hung on the last
>> pci_read_config_dword(), I haven't looked at this at all yet)
>
> Hui? How does this fix applies to a Xen guest in the first place?
>
> Please provide the output of "lspci -nn" and explain further what is
> your config with Xen.
>
>


This is dom0.

-bash-4.1# lspci -nn
00:00.0 Host bridge [0600]: ATI Technologies Inc RD890 Northbridge only
dual slot (2x16) PCI-e GFX Hydra part [1002:5a10] (rev 02)
00:00.2 Generic system peripheral [0806]: ATI Technologies Inc Device
[1002:5a23]
00:0d.0 PCI bridge [0604]: ATI Technologies Inc RD890 PCI to PCI bridge
(external gfx1 port B) [1002:5a1e]
00:11.0 SATA controller [0106]: ATI Technologies Inc SB700/SB800 SATA
Controller [AHCI mode] [1002:4391]
00:12.0 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
OHCI0 Controller [1002:4397]
00:12.1 USB Controller [0c03]: ATI Technologies Inc SB700 USB OHCI1
Controller [1002:4398]
00:12.2 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB EHCI
Controller [1002:4396]
00:13.0 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
OHCI0 Controller [1002:4397]
00:13.1 USB Controller [0c03]: ATI Technologies Inc SB700 USB OHCI1
Controller [1002:4398]
00:13.2 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB EHCI
Controller [1002:4396]
00:14.0 SMBus [0c05]: ATI Technologies Inc SBx00 SMBus Controller
[1002:4385] (rev 3d)
00:14.3 ISA bridge [0601]: ATI Technologies Inc SB700/SB800 LPC host
controller [1002:439d]
00:14.4 PCI bridge [0604]: ATI Technologies Inc SBx00 PCI to PCI Bridge
[1002:4384]
00:14.5 USB Controller [0c03]: ATI Technologies Inc SB700/SB800 USB
OHCI2 Controller [1002:4399]
00:18.0 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1600]
00:18.1 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1601]
00:18.2 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1602]
00:18.3 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1603]
00:18.4 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1604]
00:18.5 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1605]
00:19.0 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1600]
00:19.1 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1601]
00:19.2 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1602]
00:19.3 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1603]
00:19.4 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1604]
00:19.5 Host bridge [0600]: Advanced Micro Devices [AMD] Device [1022:1605]
01:04.0 VGA compatible controller [0300]: Matrox Graphics, Inc. MGA
G200eW WPCM450 [102b:0532] (rev 0a)
02:00.0 Ethernet controller [0200]: Intel Corporation 82576 Gigabit
Network Connection [8086:10c9] (rev 01)
02:00.1 Ethernet controller [0200]: Intel Corporation 82576 Gigabit
Network Connection [8086:10c9] (rev 01)
-bash-4.1#


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Boris Ostrovsky]

On 10/18/2017 09:58 AM, Christian König wrote:
> From: Christian König 
>
> Most BIOS don't enable this because of compatibility reasons.
>
> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.
>
> v2: style cleanups, increase size, add resource name, set correct flags,
> print message that windows was added
> v3: add defines for all the magic numbers, style cleanups
> v4: add some comment that the BIOS should actually allow this using
> _PRS and _SRS.
> v5: only enable this if CONFIG_PHYS_ADDR_T_64BIT is set
>
> Signed-off-by: Christian König 
> Reviewed-by: Andy Shevchenko 
> ---
>  arch/x86/pci/fixup.c | 80 
> 
>  1 file changed, 80 insertions(+)
>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 11e407489db0..7b6bd76713c5 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -618,3 +618,83 @@ static void quirk_apple_mbp_poweroff(struct pci_dev 
> *pdev)
>   dev_info(dev, "can't work around MacBook Pro poweroff issue\n");
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x8c10, 
> quirk_apple_mbp_poweroff);
> +
> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
> +
> +#define AMD_141b_MMIO_BASE(x)(0x80 + (x) * 0x8)
> +#define AMD_141b_MMIO_BASE_RE_MASK   BIT(0)
> +#define AMD_141b_MMIO_BASE_WE_MASK   BIT(1)
> +#define AMD_141b_MMIO_BASE_MMIOBASE_MASK GENMASK(31,8)
> +
> +#define AMD_141b_MMIO_LIMIT(x)   (0x84 + (x) * 0x8)
> +#define AMD_141b_MMIO_LIMIT_MMIOLIMIT_MASK   GENMASK(31,8)
> +
> +#define AMD_141b_MMIO_HIGH(x)(0x180 + (x) * 0x4)
> +#define AMD_141b_MMIO_HIGH_MMIOBASE_MASK GENMASK(7,0)
> +#define AMD_141b_MMIO_HIGH_MMIOLIMIT_SHIFT   16
> +#define AMD_141b_MMIO_HIGH_MMIOLIMIT_MASKGENMASK(23,16)
> +
> +/*
> + * The PCI Firmware Spec, rev 3.2 notes that ACPI should optionally allow
> + * configuring host bridge windows using the _PRS and _SRS methods.
> + *
> + * But this is rarely implemented, so we manually enable a large 64bit BAR 
> for
> + * PCIe device on AMD Family 15h (Models 30h-3fh) Processors here.
> + */
> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{
> + struct resource *res, *conflict;
> + u32 base, limit, high;
> + unsigned i;
> +
> + for (i = 0; i < 8; ++i) {
> + pci_read_config_dword(dev, AMD_141b_MMIO_BASE(i), );
> + pci_read_config_dword(dev, AMD_141b_MMIO_HIGH(i), );
> +
> + /* Is this slot free? */
> + if (!(base & (AMD_141b_MMIO_BASE_RE_MASK |
> +   AMD_141b_MMIO_BASE_WE_MASK)))
> + break;
> +
> + base >>= 8;
> + base |= high << 24;
> +
> + /* Abort if a slot already configures a 64bit BAR. */
> + if (base > 0x1)
> + return;
> + }
> + if (i == 8)
> + return;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + return;
> +
> + res->name = "PCI Bus :00";
> + res->flags = IORESOURCE_PREFETCH | IORESOURCE_MEM |
> + IORESOURCE_MEM_64 | IORESOURCE_WINDOW;
> + res->start = 0x1ull;
> + res->end = 0xfdull - 1;
> +
> + /* Just grab the free area behind system memory for this */
> + while ((conflict = request_resource_conflict(_resource, res)))
> + res->start = conflict->end + 1;


I get stuck in the infinite loop here.

Presumably because on a multi-socket system we succeed for the first
processor (:00:18.1) and add 'res' to iomem_resource. For
:00:19.1 we find the slot in the 'for' loop above but then we fail
to find a place to add 'res'. And with final sibling being [0 - max
possible addr] we are stuck here.

A possible solution to get out of the loop could be
if (conflict->end >= res->end) {
kfree(res);
return;
   }


but I don't know whether this is what we actually want.

This is a 2-socket

vendor_id: AuthenticAMD
cpu family: 21
model: 1
model name: AMD Opteron(TM) Processor 6272
stepping: 2


(and then it breaks differently as a Xen guest --- we hung on the last
pci_read_config_dword(), I haven't looked at this at all yet)



-boris


> +
> + dev_info(>dev, "adding root bus resource %pR\n", res);
> +
> + base = ((res->start >> 8) & AMD_141b_MMIO_BASE_MMIOBASE_MASK) |
> + AMD_141b_MMIO_BASE_RE_MASK | AMD_141b_MMIO_BASE_WE_MASK;
> + limit = ((res->end + 1) >> 8) & AMD_141b_MMIO_LIMIT_MMIOLIMIT_MASK;
> + high = ((res->start >> 40) & AMD_141b_MMIO_HIGH_MMIOBASE_MASK) |
> + res->end + 1) >> 40) << AMD_141b_MMIO_HIGH_MMIOLIMIT_SHIFT)
> +  & AMD_141b_MMIO_HIGH_MMIOLIMIT_MASK);
> +
> + pci_write_config_dword(dev, AMD_141b_MMIO_HIGH(i), high);
> +