Re: [RFC PATCH v1 10/25] hw/xen: Add gnttab operations to allow redirection to internal emulation

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 02/03/2023 15:34, David Woodhouse wrote:
> From: David Woodhouse <dwmw@xxxxxxxxxxxx>
>
> Move the existing code using libxengnttab to xen-operations.c and allow
> the operations to be redirected so that we can add emulation of grant
> table mapping for backend drivers.
>
> In emulation, mapping more than one grant ref to be virtually contiguous
> would be fairly difficult. The best way to do it might be to make the
> ram_block mappings actually backed by a file (shmem or a deleted file,
> perhaps) so that we can have multiple *shared* mappings of it. But that
> would be fairly intrusive.
>
> Making the backend drivers cope with page *lists* instead of expecting
> the mapping to be contiguous is also non-trivial, since some structures
> would actually *cross* page boundaries (e.g. the 32-bit blkif responses
> which are 12 bytes).
>
> So for now, we'll support only single-page mappings in emulation. Add a
> XEN_GNTTAB_OP_FEATURE_MAP_MULTIPLE flag to indicate that the native Xen
> implementation *does* support multi-page maps, and a helper function to
> query it.
>
> Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
[snip]
> @@ -65,7 +195,34 @@ struct evtchn_backend_ops libxenevtchn_backend_ops = {
>       .pending = xenevtchn_pending,
>   };
>   
> +static xengnttab_handle *libxengnttab_backend_open(void)
> +{
> +    return xengnttab_open(NULL, 0);
> +}
> +
> +
> +static struct gnttab_backend_ops libxengnttab_backend_ops = {
> +    .features = XEN_GNTTAB_OP_FEATURE_MAP_MULTIPLE,
> +    .open = libxengnttab_backend_open,
> +    .close = xengnttab_close,
> +    .grant_copy = libxengnttab_fallback_grant_copy,
> +    .set_max_grants = xengnttab_set_max_grants,
> +    .map_refs = xengnttab_map_domain_grant_refs,
> +    .unmap = xengnttab_unmap,
> +};
> +
>   void setup_xen_backend_ops(void)
>   {
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40800
> +    xengnttab_handle *xgt = xengnttab_open(NULL, 0);
> +
> +    if (xgt) {
> +        if (xengnttab_grant_copy(xgt, 0, NULL) == 0) {
> +            xen_gnttab_ops->grant_copy = libxengnttab_backend_grant_copy;
As we found out, this ^^^ is dereferencing a NULL pointer. Switching 
'xen_gnttab_ops->' for the obviously intended 
'libxengnttab_backend_ops.' fixes the problem.
> +        }
> +        xengnttab_close(xgt);
> +    }
> +#endif
>       xen_evtchn_ops = &libxenevtchn_backend_ops;
> +    xen_gnttab_ops = &libxengnttab_backend_ops;
With that rectified...

Reviewed-by: Paul Durrant <paul@xxxxxxx>

>   }