Re: [PATCH v3 3/3] xen: don't require virtio with grants for non-PV guests

[Juergen Gross <jgross@xxxxxxxx>]

On 22.06.22 11:03, Oleksandr wrote:
> On 22.06.22 09:38, Juergen Gross wrote:
>
> Hello Juergen
>
> > Commit fa1f57421e0b ("xen/virtio: Enable restricted memory access using
> > Xen grant mappings") introduced a new requirement for using virtio
> > devices: the backend now needs to support the VIRTIO_F_ACCESS_PLATFORM
> > feature.
> >
> > This is an undue requirement for non-PV guests, as those can be operated
> > with existing backends without any problem, as long as those backends
> > are running in dom0.
> >
> > Per default allow virtio devices without grant support for non-PV
> > guests.
> >
> > On Arm require VIRTIO_F_ACCESS_PLATFORM for devices having been listed
> > in the device tree to use grants.
> >
> > Add a new config item to always force use of grants for virtio.
> >
> > Fixes: fa1f57421e0b ("xen/virtio: Enable restricted memory access using Xen 
> > grant mappings")
> > Reported-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - remove command line parameter (Christoph Hellwig)
> > V3:
> > - rebase to callback method
>
> Patch looks good, just one NIT ...
>
>
> > ---
> >   arch/arm/xen/enlighten.c     |  4 +++-
> >   arch/x86/xen/enlighten_hvm.c |  4 +++-
> >   arch/x86/xen/enlighten_pv.c  |  5 ++++-
> >   drivers/xen/Kconfig          |  9 +++++++++
> >   drivers/xen/grant-dma-ops.c  | 10 ++++++++++
> >   include/xen/xen-ops.h        |  6 ++++++
> >   include/xen/xen.h            |  8 --------
> >   7 files changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index 1f9c3ba32833..93c8ccbf2982 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -34,6 +34,7 @@
> >   #include <linux/timekeeping.h>
> >   #include <linux/timekeeper_internal.h>
> >   #include <linux/acpi.h>
> > +#include <linux/virtio_anchor.h>
> >   #include <linux/mm.h>
> > @@ -443,7 +444,8 @@ static int __init xen_guest_init(void)
> >       if (!xen_domain())
> >           return 0;
> > -    xen_set_restricted_virtio_memory_access();
> > +    if (IS_ENABLED(CONFIG_XEN_VIRTIO))
> > +        virtio_set_mem_acc_cb(xen_virtio_mem_acc);
> >       if (!acpi_disabled)
> >           xen_acpi_guest_init();
> > diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
> > index 8b71b1dd7639..28762f800596 100644
> > --- a/arch/x86/xen/enlighten_hvm.c
> > +++ b/arch/x86/xen/enlighten_hvm.c
> > @@ -4,6 +4,7 @@
> >   #include <linux/cpu.h>
> >   #include <linux/kexec.h>
> >   #include <linux/memblock.h>
> > +#include <linux/virtio_anchor.h>
> >   #include <xen/features.h>
> >   #include <xen/events.h>
> > @@ -195,7 +196,8 @@ static void __init xen_hvm_guest_init(void)
> >       if (xen_pv_domain())
> >           return;
> > -    xen_set_restricted_virtio_memory_access();
> > +    if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
> > +        virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
> >       init_hvm_pv_info();
> > diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> > index e3297b15701c..5aaae8a77f55 100644
> > --- a/arch/x86/xen/enlighten_pv.c
> > +++ b/arch/x86/xen/enlighten_pv.c
> > @@ -31,6 +31,7 @@
> >   #include <linux/gfp.h>
> >   #include <linux/edd.h>
> >   #include <linux/reboot.h>
> > +#include <linux/virtio_anchor.h>
> >   #include <xen/xen.h>
> >   #include <xen/events.h>
> > @@ -109,7 +110,9 @@ static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
> >   static void __init xen_pv_init_platform(void)
> >   {
> > -    xen_set_restricted_virtio_memory_access();
> > +    /* PV guests can't operate virtio devices without grants. */
> > +    if (IS_ENABLED(CONFIG_XEN_VIRTIO))
> > +        virtio_set_mem_acc_cb(virtio_require_restricted_mem_acc);
> >       populate_extra_pte(fix_to_virt(FIX_PARAVIRT_BOOTMAP));
> > diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
> > index bfd5f4f706bc..a65bd92121a5 100644
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -355,4 +355,13 @@ config XEN_VIRTIO
> >         If in doubt, say n.
> > +config XEN_VIRTIO_FORCE_GRANT
> > +    bool "Require Xen virtio support to use grants"
> > +    depends on XEN_VIRTIO
> > +    help
> > +      Require virtio for Xen guests to use grant mappings.
> > +      This will avoid the need to give the backend the right to map all
> > +      of the guest memory. This will need support on the backend side
> > +      (e.g. qemu or kernel, depending on the virtio device types used).
> > +
> >   endmenu
> > diff --git a/drivers/xen/grant-dma-ops.c b/drivers/xen/grant-dma-ops.c
> > index fc0142484001..8973fc1e9ccc 100644
> > --- a/drivers/xen/grant-dma-ops.c
> > +++ b/drivers/xen/grant-dma-ops.c
> > @@ -12,6 +12,8 @@
> >   #include <linux/of.h>
> >   #include <linux/pfn.h>
> >   #include <linux/xarray.h>
> > +#include <linux/virtio_anchor.h>
> > +#include <linux/virtio.h>
> >   #include <xen/xen.h>
> >   #include <xen/xen-ops.h>
> >   #include <xen/grant_table.h>
> > @@ -287,6 +289,14 @@ bool xen_is_grant_dma_device(struct device *dev)
> >       return has_iommu;
> >   }
> > +bool xen_virtio_mem_acc(struct virtio_device *dev)
> > +{
> > +    if (IS_ENABLED(CONFIG_XEN_VIRTIO_FORCE_GRANT))
> > +        return true;
> > +
> > +    return xen_is_grant_dma_device(dev->dev.parent);
> > +}
>
>     ... I am thinking would it be better to move this to xen/xen-ops.h as 
> grant-dma-ops.c is generic (not only for virtio, although the virtio is the 
> first use-case)
I dislike using a function marked as inline in a function vector.

We could add another module "xen-virtio" for this purpose, but this seems
to be overkill.

I think we should just leave it here and move it later in case more real
virtio dependent stuff is being added.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature