[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




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];
(END)




                    } else {
                            to_copy = XEN_NUM_GREFS_PER_PAGE;
                            page_dir->gref_dir_next_page =
    buf->grefs[i + 1];
-- 2.34.1




--
Regards,

Oleksandr Tyshchenko

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.