Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF

From: Juergen Gross
Date: Mon May 02 2022 - 09:28:25 EST

On 28.04.22 20: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...

Hmm, that's unfortunate.

https://elixir.bootlin.com/linux/v5.18-rc4/source/include/xen/interface/io/sndif.h#L650 <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.

Thanks for finding this.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature