* [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly @ 2022-10-07 13:27 Oleksandr Tyshchenko 2022-10-07 15:10 ` Juergen Gross 2022-10-08 11:08 ` Xenia Ragiadakou 0 siblings, 2 replies; 6+ messages in thread From: Oleksandr Tyshchenko @ 2022-10-07 13:27 UTC (permalink / raw) To: xen-devel, linux-kernel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Juergen Gross, Xenia Ragiadakou From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Passed to xen_grant_dma_map_page() offset in the page can be > PAGE_SIZE even if the guest uses the same page granularity as Xen (4KB). Before current patch, if such case happened we ended up providing grants for the whole region in xen_grant_dma_map_page() which was really unnecessary. The more, we ended up not releasing all grants which represented that region in xen_grant_dma_unmap_page(). Current patch updates the code to be able to deal with such cases. Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> --- Cc: Juergen Gross <jgross@suse.com> Cc: Xenia Ragiadakou <burzalodowa@gmail.com> Depens on: https://lore.kernel.org/xen-devel/20221005174823.1800761-1-olekstysh@gmail.com/ Should go in only after that series. --- drivers/xen/grant-dma-ops.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c index c66f56d24013..1385f0e686fe 100644 --- a/drivers/xen/grant-dma-ops.c +++ b/drivers/xen/grant-dma-ops.c @@ -168,7 +168,9 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page, unsigned long attrs) { struct xen_grant_dma_data *data; - unsigned int i, n_pages = PFN_UP(offset + size); + unsigned long dma_offset = offset_in_page(offset), + gfn_offset = PFN_DOWN(offset); + unsigned int i, n_pages = PFN_UP(dma_offset + size); grant_ref_t grant; dma_addr_t dma_handle; @@ -187,10 +189,10 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page, for (i = 0; i < n_pages; i++) { gnttab_grant_foreign_access_ref(grant + i, data->backend_domid, - xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE); + xen_page_to_gfn(page) + i + gfn_offset, dir == DMA_TO_DEVICE); } - dma_handle = grant_to_dma(grant) + offset; + dma_handle = grant_to_dma(grant) + dma_offset; return dma_handle; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly 2022-10-07 13:27 [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly Oleksandr Tyshchenko @ 2022-10-07 15:10 ` Juergen Gross 2022-10-08 11:08 ` Xenia Ragiadakou 1 sibling, 0 replies; 6+ messages in thread From: Juergen Gross @ 2022-10-07 15:10 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel, linux-kernel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Xenia Ragiadakou [-- Attachment #1.1.1: Type: text/plain, Size: 747 bytes --] On 07.10.22 15:27, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Passed to xen_grant_dma_map_page() offset in the page > can be > PAGE_SIZE even if the guest uses the same page granularity > as Xen (4KB). > > Before current patch, if such case happened we ended up providing > grants for the whole region in xen_grant_dma_map_page() which > was really unnecessary. The more, we ended up not releasing all > grants which represented that region in xen_grant_dma_unmap_page(). > > Current patch updates the code to be able to deal with such cases. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 3149 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly 2022-10-07 13:27 [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly Oleksandr Tyshchenko 2022-10-07 15:10 ` Juergen Gross @ 2022-10-08 11:08 ` Xenia Ragiadakou 2022-10-08 12:52 ` Oleksandr Tyshchenko 1 sibling, 1 reply; 6+ messages in thread From: Xenia Ragiadakou @ 2022-10-08 11:08 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel, linux-kernel Cc: Oleksandr Tyshchenko, Stefano Stabellini, Juergen Gross On 10/7/22 16:27, Oleksandr Tyshchenko wrote: Hi Oleksandr > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Passed to xen_grant_dma_map_page() offset in the page > can be > PAGE_SIZE even if the guest uses the same page granularity > as Xen (4KB). > > Before current patch, if such case happened we ended up providing > grants for the whole region in xen_grant_dma_map_page() which > was really unnecessary. The more, we ended up not releasing all > grants which represented that region in xen_grant_dma_unmap_page(). > > Current patch updates the code to be able to deal with such cases. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > Cc: Juergen Gross <jgross@suse.com> > Cc: Xenia Ragiadakou <burzalodowa@gmail.com> > > Depens on: > https://lore.kernel.org/xen-devel/20221005174823.1800761-1-olekstysh@gmail.com/ > > Should go in only after that series. > --- > drivers/xen/grant-dma-ops.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c > index c66f56d24013..1385f0e686fe 100644 > --- a/drivers/xen/grant-dma-ops.c > +++ b/drivers/xen/grant-dma-ops.c > @@ -168,7 +168,9 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page, > unsigned long attrs) > { > struct xen_grant_dma_data *data; > - unsigned int i, n_pages = PFN_UP(offset + size); > + unsigned long dma_offset = offset_in_page(offset), > + gfn_offset = PFN_DOWN(offset); > + unsigned int i, n_pages = PFN_UP(dma_offset + size); IIUC, the above with a later patch will become: dma_offset = xen_offset_in_page(offset) gfn_offset = XEN_PFN_DOWN(offset) n_pages = XEN_PFN_UP(dma_offset + size) > grant_ref_t grant; > dma_addr_t dma_handle; > > @@ -187,10 +189,10 @@ static dma_addr_t xen_grant_dma_map_page(struct device *dev, struct page *page, > > for (i = 0; i < n_pages; i++) { > gnttab_grant_foreign_access_ref(grant + i, data->backend_domid, > - xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE); > + xen_page_to_gfn(page) + i + gfn_offset, dir == DMA_TO_DEVICE); Here, why the pfn is not calculated before passing it to pfn_to_gfn()? I mean sth like pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i) > } > > - dma_handle = grant_to_dma(grant) + offset; > + dma_handle = grant_to_dma(grant) + dma_offset; > > return dma_handle; > } -- Xenia ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly 2022-10-08 11:08 ` Xenia Ragiadakou @ 2022-10-08 12:52 ` Oleksandr Tyshchenko 2022-10-08 12:59 ` Xenia Ragiadakou 0 siblings, 1 reply; 6+ messages in thread From: Oleksandr Tyshchenko @ 2022-10-08 12:52 UTC (permalink / raw) To: Xenia Ragiadakou, xen-devel, linux-kernel Cc: Stefano Stabellini, Juergen Gross, Oleksandr Tyshchenko On 08.10.22 14:08, Xenia Ragiadakou wrote: Hello Xenia > > On 10/7/22 16:27, Oleksandr Tyshchenko wrote: > > Hi Oleksandr > >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Passed to xen_grant_dma_map_page() offset in the page >> can be > PAGE_SIZE even if the guest uses the same page granularity >> as Xen (4KB). >> >> Before current patch, if such case happened we ended up providing >> grants for the whole region in xen_grant_dma_map_page() which >> was really unnecessary. The more, we ended up not releasing all >> grants which represented that region in xen_grant_dma_unmap_page(). >> >> Current patch updates the code to be able to deal with such cases. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> Cc: Juergen Gross <jgross@suse.com> >> Cc: Xenia Ragiadakou <burzalodowa@gmail.com> >> >> Depens on: >> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221005174823.1800761-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!xnkNaKpfZ4LssQJcJs_J91KERZKMP2Rd-xEdBqXNXJ8GyCXJ0gkRer1elVYfxOWtwN_FOl9tVieDWlfN-UZaHQsyLMhA$ >> [lore[.]kernel[.]org] >> >> Should go in only after that series. >> --- >> drivers/xen/grant-dma-ops.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c >> index c66f56d24013..1385f0e686fe 100644 >> --- a/drivers/xen/grant-dma-ops.c >> +++ b/drivers/xen/grant-dma-ops.c >> @@ -168,7 +168,9 @@ static dma_addr_t xen_grant_dma_map_page(struct >> device *dev, struct page *page, >> unsigned long attrs) >> { >> struct xen_grant_dma_data *data; >> - unsigned int i, n_pages = PFN_UP(offset + size); >> + unsigned long dma_offset = offset_in_page(offset), >> + gfn_offset = PFN_DOWN(offset); >> + unsigned int i, n_pages = PFN_UP(dma_offset + size); > > IIUC, the above with a later patch will become: > > dma_offset = xen_offset_in_page(offset) > gfn_offset = XEN_PFN_DOWN(offset) > n_pages = XEN_PFN_UP(dma_offset + size) If saying "later" patch you meant "xen/virtio: Convert PAGE_SIZE/PAGE_SHIFT/PFN_UP to Xen counterparts" then yes, exactly. > > >> grant_ref_t grant; >> dma_addr_t dma_handle; >> @@ -187,10 +189,10 @@ static dma_addr_t >> xen_grant_dma_map_page(struct device *dev, struct page *page, >> for (i = 0; i < n_pages; i++) { >> gnttab_grant_foreign_access_ref(grant + i, >> data->backend_domid, >> - xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE); >> + xen_page_to_gfn(page) + i + gfn_offset, dir == >> DMA_TO_DEVICE); > > Here, why the pfn is not calculated before passing it to pfn_to_gfn()? > I mean sth like pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i) The gfn_offset is just a const value here, which just means how many gfns we should skip. But ... ... I think, I get your point. So, if the region which is contiguous in pfn might be non-contiguous in gfn (which seems to be the case for x86's PV, but I may mistake) we should indeed use open-coded construction "pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i)". And the gfn_offset should be renamed to pfn_offset then. Correct? Thanks. > >> } >> - dma_handle = grant_to_dma(grant) + offset; >> + dma_handle = grant_to_dma(grant) + dma_offset; >> return dma_handle; >> } > -- Regards, Oleksandr Tyshchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly 2022-10-08 12:52 ` Oleksandr Tyshchenko @ 2022-10-08 12:59 ` Xenia Ragiadakou 2022-10-08 13:21 ` Oleksandr Tyshchenko 0 siblings, 1 reply; 6+ messages in thread From: Xenia Ragiadakou @ 2022-10-08 12:59 UTC (permalink / raw) To: Oleksandr Tyshchenko, xen-devel, linux-kernel Cc: Stefano Stabellini, Juergen Gross, Oleksandr Tyshchenko On 10/8/22 15:52, Oleksandr Tyshchenko wrote: > > On 08.10.22 14:08, Xenia Ragiadakou wrote: > > Hello Xenia > >> >> On 10/7/22 16:27, Oleksandr Tyshchenko wrote: >> >> Hi Oleksandr >> >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> Passed to xen_grant_dma_map_page() offset in the page >>> can be > PAGE_SIZE even if the guest uses the same page granularity >>> as Xen (4KB). >>> >>> Before current patch, if such case happened we ended up providing >>> grants for the whole region in xen_grant_dma_map_page() which >>> was really unnecessary. The more, we ended up not releasing all >>> grants which represented that region in xen_grant_dma_unmap_page(). >>> >>> Current patch updates the code to be able to deal with such cases. >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> Cc: Juergen Gross <jgross@suse.com> >>> Cc: Xenia Ragiadakou <burzalodowa@gmail.com> >>> >>> Depens on: >>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221005174823.1800761-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!xnkNaKpfZ4LssQJcJs_J91KERZKMP2Rd-xEdBqXNXJ8GyCXJ0gkRer1elVYfxOWtwN_FOl9tVieDWlfN-UZaHQsyLMhA$ >>> [lore[.]kernel[.]org] >>> >>> Should go in only after that series. >>> --- >>> drivers/xen/grant-dma-ops.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c >>> index c66f56d24013..1385f0e686fe 100644 >>> --- a/drivers/xen/grant-dma-ops.c >>> +++ b/drivers/xen/grant-dma-ops.c >>> @@ -168,7 +168,9 @@ static dma_addr_t xen_grant_dma_map_page(struct >>> device *dev, struct page *page, >>> unsigned long attrs) >>> { >>> struct xen_grant_dma_data *data; >>> - unsigned int i, n_pages = PFN_UP(offset + size); >>> + unsigned long dma_offset = offset_in_page(offset), >>> + gfn_offset = PFN_DOWN(offset); >>> + unsigned int i, n_pages = PFN_UP(dma_offset + size); >> >> IIUC, the above with a later patch will become: >> >> dma_offset = xen_offset_in_page(offset) >> gfn_offset = XEN_PFN_DOWN(offset) >> n_pages = XEN_PFN_UP(dma_offset + size) > > > If saying "later" patch you meant "xen/virtio: Convert > PAGE_SIZE/PAGE_SHIFT/PFN_UP to Xen counterparts" then yes, exactly. Ah ok, I see. >> >> >>> grant_ref_t grant; >>> dma_addr_t dma_handle; >>> @@ -187,10 +189,10 @@ static dma_addr_t >>> xen_grant_dma_map_page(struct device *dev, struct page *page, >>> for (i = 0; i < n_pages; i++) { >>> gnttab_grant_foreign_access_ref(grant + i, >>> data->backend_domid, >>> - xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE); >>> + xen_page_to_gfn(page) + i + gfn_offset, dir == >>> DMA_TO_DEVICE); >> >> Here, why the pfn is not calculated before passing it to pfn_to_gfn()? >> I mean sth like pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i) > > The gfn_offset is just a const value here, which just means how many > gfns we should skip. But ... > > ... I think, I get your point. So, if the region which is contiguous in > pfn might be non-contiguous in gfn (which seems to be the case for x86's > PV, but I may mistake) we should indeed use open-coded > > construction "pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i)". And > the gfn_offset should be renamed to pfn_offset then. > > > Correct? Yes, that 's what I had in mind unless I 'm missing sth. >> >>> } >>> - dma_handle = grant_to_dma(grant) + offset; >>> + dma_handle = grant_to_dma(grant) + dma_offset; >>> return dma_handle; >>> } >> -- Xenia ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly 2022-10-08 12:59 ` Xenia Ragiadakou @ 2022-10-08 13:21 ` Oleksandr Tyshchenko 0 siblings, 0 replies; 6+ messages in thread From: Oleksandr Tyshchenko @ 2022-10-08 13:21 UTC (permalink / raw) To: Xenia Ragiadakou, xen-devel, linux-kernel Cc: Stefano Stabellini, Juergen Gross, Oleksandr Tyshchenko On 08.10.22 15:59, Xenia Ragiadakou wrote: Hello Xenia > > On 10/8/22 15:52, Oleksandr Tyshchenko wrote: >> >> On 08.10.22 14:08, Xenia Ragiadakou wrote: >> >> Hello Xenia >> >>> >>> On 10/7/22 16:27, Oleksandr Tyshchenko wrote: >>> >>> Hi Oleksandr >>> >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> Passed to xen_grant_dma_map_page() offset in the page >>>> can be > PAGE_SIZE even if the guest uses the same page granularity >>>> as Xen (4KB). >>>> >>>> Before current patch, if such case happened we ended up providing >>>> grants for the whole region in xen_grant_dma_map_page() which >>>> was really unnecessary. The more, we ended up not releasing all >>>> grants which represented that region in xen_grant_dma_unmap_page(). >>>> >>>> Current patch updates the code to be able to deal with such cases. >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> --- >>>> Cc: Juergen Gross <jgross@suse.com> >>>> Cc: Xenia Ragiadakou <burzalodowa@gmail.com> >>>> >>>> Depens on: >>>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20221005174823.1800761-1-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!xnkNaKpfZ4LssQJcJs_J91KERZKMP2Rd-xEdBqXNXJ8GyCXJ0gkRer1elVYfxOWtwN_FOl9tVieDWlfN-UZaHQsyLMhA$ >>>> >>>> [lore[.]kernel[.]org] >>>> >>>> Should go in only after that series. >>>> --- >>>> drivers/xen/grant-dma-ops.c | 8 +++++--- >>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c >>>> index c66f56d24013..1385f0e686fe 100644 >>>> --- a/drivers/xen/grant-dma-ops.c >>>> +++ b/drivers/xen/grant-dma-ops.c >>>> @@ -168,7 +168,9 @@ static dma_addr_t xen_grant_dma_map_page(struct >>>> device *dev, struct page *page, >>>> unsigned long attrs) >>>> { >>>> struct xen_grant_dma_data *data; >>>> - unsigned int i, n_pages = PFN_UP(offset + size); >>>> + unsigned long dma_offset = offset_in_page(offset), >>>> + gfn_offset = PFN_DOWN(offset); >>>> + unsigned int i, n_pages = PFN_UP(dma_offset + size); >>> >>> IIUC, the above with a later patch will become: >>> >>> dma_offset = xen_offset_in_page(offset) >>> gfn_offset = XEN_PFN_DOWN(offset) >>> n_pages = XEN_PFN_UP(dma_offset + size) >> >> >> If saying "later" patch you meant "xen/virtio: Convert >> PAGE_SIZE/PAGE_SHIFT/PFN_UP to Xen counterparts" then yes, exactly. > > Ah ok, I see. > >>> >>> >>>> grant_ref_t grant; >>>> dma_addr_t dma_handle; >>>> @@ -187,10 +189,10 @@ static dma_addr_t >>>> xen_grant_dma_map_page(struct device *dev, struct page *page, >>>> for (i = 0; i < n_pages; i++) { >>>> gnttab_grant_foreign_access_ref(grant + i, >>>> data->backend_domid, >>>> - xen_page_to_gfn(page) + i, dir == DMA_TO_DEVICE); >>>> + xen_page_to_gfn(page) + i + gfn_offset, dir == >>>> DMA_TO_DEVICE); >>> >>> Here, why the pfn is not calculated before passing it to pfn_to_gfn()? >>> I mean sth like pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i) >> >> The gfn_offset is just a const value here, which just means how many >> gfns we should skip. But ... >> >> ... I think, I get your point. So, if the region which is contiguous in >> pfn might be non-contiguous in gfn (which seems to be the case for x86's >> PV, but I may mistake) we should indeed use open-coded >> >> construction "pfn_to_gfn(page_to_xen_pfn(page) + gfn_offset + i)". And >> the gfn_offset should be renamed to pfn_offset then. >> >> >> Correct? > > Yes, that 's what I had in mind unless I 'm missing sth. ok, thanks for confirming. So I will create V2 then. > >>> >>>> } >>>> - dma_handle = grant_to_dma(grant) + offset; >>>> + dma_handle = grant_to_dma(grant) + dma_offset; >>>> return dma_handle; >>>> } >>> > -- Regards, Oleksandr Tyshchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-08 13:22 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-07 13:27 [PATCH] xen/virtio: Handle cases when page offset > PAGE_SIZE properly Oleksandr Tyshchenko 2022-10-07 15:10 ` Juergen Gross 2022-10-08 11:08 ` Xenia Ragiadakou 2022-10-08 12:52 ` Oleksandr Tyshchenko 2022-10-08 12:59 ` Xenia Ragiadakou 2022-10-08 13:21 ` Oleksandr Tyshchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).