Messages in this thread | | | Subject | Re: [PATCH v2 08/19] xen/shbuf: switch xen-front-pgdir-shbuf to use INVALID_GRANT_REF | From | Oleksandr <> | Date | Mon, 2 May 2022 16:52:29 +0300 |
| |
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@suse.com >>> <mailto:jgross@suse.com>> 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@suse.com >>> <mailto:jgross@suse.com>> >>> --- >>> 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
| |