Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF
From: Oleksandr
Date: Mon May 02 2022 - 09:52:36 EST
On 02.05.22 16:31, Juergen Gross wrote:
Hello Juergen
On 29.04.22 17:28, Oleksandr wrote:
Hello Juergen
On 28.04.22 21:03, Oleksandr Tyshchenko wrote:
On Thu, Apr 28, 2022 at 11:28 AM Juergen Gross <jgross@xxxxxxxx
<mailto:jgross@xxxxxxxx>> wrote:
Hello Juergen
[sorry for the possible format issue]
Instead of using a private macro for an invalid grant reference use
the common one.
Signed-off-by: Juergen Gross <jgross@xxxxxxxx
<mailto:jgross@xxxxxxxx>>
---
drivers/xen/xen-front-pgdir-shbuf.c | 17 ++++-------------
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
b/drivers/xen/xen-front-pgdir-shbuf.c
index a959dee21134..fa2921d4fbfc 100644
--- a/drivers/xen/xen-front-pgdir-shbuf.c
+++ b/drivers/xen/xen-front-pgdir-shbuf.c
@@ -21,15 +21,6 @@
#include <xen/xen-front-pgdir-shbuf.h>
-#ifndef GRANT_INVALID_REF
-/*
- * FIXME: usage of grant reference 0 as invalid grant reference:
- * grant reference 0 is valid, but never exposed to a PV driver,
- * because of the fact it is already in use/reserved by the PV
console.
- */
-#define GRANT_INVALID_REF 0
-#endif
-
/**
* This structure represents the structure of a shared page
* that contains grant references to the pages of the shared
@@ -83,7 +74,7 @@ grant_ref_t
xen_front_pgdir_shbuf_get_dir_start(struct xen_front_pgdir_shbuf
*buf)
{
if (!buf->grefs)
- return GRANT_INVALID_REF;
+ return INVALID_GRANT_REF;
return buf->grefs[0];
}
@@ -142,7 +133,7 @@ void xen_front_pgdir_shbuf_free(struct
xen_front_pgdir_shbuf *buf)
int i;
for (i = 0; i < buf->num_grefs; i++)
- if (buf->grefs[i] != GRANT_INVALID_REF)
+ if (buf->grefs[i] != INVALID_GRANT_REF)
gnttab_end_foreign_access(buf->grefs[i], 0UL);
}
kfree(buf->grefs);
@@ -355,7 +346,7 @@ static void backend_fill_page_dir(struct
xen_front_pgdir_shbuf *buf)
}
/* Last page must say there is no more pages. */
page_dir = (struct xen_page_directory *)ptr;
- page_dir->gref_dir_next_page = GRANT_INVALID_REF;
+ page_dir->gref_dir_next_page = INVALID_GRANT_REF;
}
/**
@@ -384,7 +375,7 @@ static void guest_fill_page_dir(struct
xen_front_pgdir_shbuf *buf)
if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
to_copy = grefs_left;
- page_dir->gref_dir_next_page =
GRANT_INVALID_REF;
+ page_dir->gref_dir_next_page =
INVALID_GRANT_REF;
I faced an issue with testing PV Sound with the current series.
root@salvator-x-h3-4x2g-xt-domu:~# aplay /media/MoodyLoop.wav
Playing WAVE '/media/MoodyLoop.wav' : Signed 16 bit Little Endian,
Rate 44100 Hz, Stereo
(XEN) common/grant_table.c:1053:d1v2 Bad ref 0xffffffff for d6
Here we have an interesting situation. PV Sound frontend uses this
xen-front-pgdir-shbuf framework. Technically, this patch changes
page_dir->gref_dir_next_page (reference to the next page describing
page directory) from 0 to 0xffffffff here.
#define INVALID_GRANT_REF ((grant_ref_t)-1)
But according to the protocol (sndif.h), "0" means that there are no
more pages in the list and the user space backend expects only that
value. So receiving 0xffffffff it assumes there are pages in the
list and trying to process...
https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650
I think, the same is relevant to backend_fill_page_dir() as well.
In addition to what I said yesterday:
PV Display also uses this xen-front-pgdir-shbuf framework. It's
protocol (displif.h) also mentions the same as sndif.h if the context
of gref_dir_next_page:
* gref_dir_next_page - grant_ref_t, reference to the next page
describing
* page directory. Must be 0 if there are no more pages in the list.
With that local change both PV devices work in my environment.
diff --git a/drivers/xen/xen-front-pgdir-shbuf.c
b/drivers/xen/xen-front-pgdir-shbuf.c
index fa2921d..ad4a88e 100644
--- a/drivers/xen/xen-front-pgdir-shbuf.c
+++ b/drivers/xen/xen-front-pgdir-shbuf.c
@@ -346,7 +346,7 @@ static void backend_fill_page_dir(struct
xen_front_pgdir_shbuf *buf)
}
/* Last page must say there is no more pages. */
page_dir = (struct xen_page_directory *)ptr;
- page_dir->gref_dir_next_page = INVALID_GRANT_REF;
+ page_dir->gref_dir_next_page = 0;
}
/**
@@ -375,7 +375,7 @@ static void guest_fill_page_dir(struct
xen_front_pgdir_shbuf *buf)
if (grefs_left <= XEN_NUM_GREFS_PER_PAGE) {
to_copy = grefs_left;
- page_dir->gref_dir_next_page =
INVALID_GRANT_REF;
+ page_dir->gref_dir_next_page = 0;
} else {
to_copy = XEN_NUM_GREFS_PER_PAGE;
page_dir->gref_dir_next_page = buf->grefs[i
+ 1];
I think I'll introduce a proper define for that purpose.
I think it would be the best option.
Juergen
--
Regards,
Oleksandr Tyshchenko