[v1] xen-arm: add support for virtio-pci

[PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

Posted by Volodymyr Babchuk 2 years, 3 months ago

Add option to preserve owner when creating an entry in Xen Store. This
may be needed in cases when Qemu is working as device model in a
domain that is Domain-0, e.g. in driver domain.

"owner" parameter for qemu_xen_xs_create() function can have special
value XS_PRESERVE_OWNER, which will make specific implementation to
get original owner of an entry and pass it back to
set_permissions() call.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
 hw/xen/xen-operations.c          | 12 ++++++++++++
 include/hw/xen/xen_backend_ops.h |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 660d0b72f9..7b894a9884 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
         return false;
     }
 
+    if (owner == XS_PRESERVE_OWNER) {
+        GList *perms;
+        char letter;
+
+        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
+        if (err) {
+            errno = err;
+            return false;
+        }
+
+        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {
+            errno = EFAULT;
+            g_list_free_full(perms, g_free);
+            return false;
+        }
+        g_list_free_full(perms, g_free);
+    }
+
     perms_list = g_list_append(perms_list,
                                xs_perm_as_string(XS_PERM_NONE, owner));
     perms_list = g_list_append(perms_list,
diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
index e00983ec44..1df59b3c08 100644
--- a/hw/xen/xen-operations.c
+++ b/hw/xen/xen-operations.c
@@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
         return false;
     }
 
+    if (owner == XS_PRESERVE_OWNER) {
+        struct xs_permissions *tmp;
+        unsigned int num;
+
+        tmp = xs_get_permissions(h->xsh, 0, path, &num);
+        if (tmp == NULL) {
+            return false;
+        }
+        perms_list[0].id = tmp[0].id;
+        free(tmp);
+    }
+
     return xs_set_permissions(h->xsh, t, path, perms_list,
                               ARRAY_SIZE(perms_list));
 }
diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
index 90cca85f52..273e414559 100644
--- a/include/hw/xen/xen_backend_ops.h
+++ b/include/hw/xen/xen_backend_ops.h
@@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
 #define XS_PERM_READ  0x01
 #define XS_PERM_WRITE 0x02
 
+#define XS_PRESERVE_OWNER        0xFFFE
+
 struct xenstore_backend_ops {
     struct qemu_xs_handle *(*open)(void);
     void (*close)(struct qemu_xs_handle *h);
-- 
2.42.0

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

Posted by Paul Durrant 2 years, 3 months ago

On 10/11/2023 15:42, Volodymyr Babchuk wrote:
> Add option to preserve owner when creating an entry in Xen Store. This
> may be needed in cases when Qemu is working as device model in a
> domain that is Domain-0, e.g. in driver domain.
> 
> "owner" parameter for qemu_xen_xs_create() function can have special
> value XS_PRESERVE_OWNER, which will make specific implementation to
> get original owner of an entry and pass it back to
> set_permissions() call.
> 

If QEMU is running in a driver domain then it should know whether the 
domid of the domain it is running in and use that. Yes, it is hardcoded 
to 0 at the moment but surely a backend domid (which defaults to 0) 
could be passed on the command line?

   Paul

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>   hw/xen/xen-operations.c          | 12 ++++++++++++
>   include/hw/xen/xen_backend_ops.h |  2 ++
>   3 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> index 660d0b72f9..7b894a9884 100644
> --- a/hw/i386/kvm/xen_xenstore.c
> +++ b/hw/i386/kvm/xen_xenstore.c
> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>           return false;
>       }
>   
> +    if (owner == XS_PRESERVE_OWNER) {
> +        GList *perms;
> +        char letter;
> +
> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
> +        if (err) {
> +            errno = err;
> +            return false;
> +        }
> +
> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {
> +            errno = EFAULT;
> +            g_list_free_full(perms, g_free);
> +            return false;
> +        }
> +        g_list_free_full(perms, g_free);
> +    }
> +
>       perms_list = g_list_append(perms_list,
>                                  xs_perm_as_string(XS_PERM_NONE, owner));
>       perms_list = g_list_append(perms_list,
> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
> index e00983ec44..1df59b3c08 100644
> --- a/hw/xen/xen-operations.c
> +++ b/hw/xen/xen-operations.c
> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>           return false;
>       }
>   
> +    if (owner == XS_PRESERVE_OWNER) {
> +        struct xs_permissions *tmp;
> +        unsigned int num;
> +
> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> +        if (tmp == NULL) {
> +            return false;
> +        }
> +        perms_list[0].id = tmp[0].id;
> +        free(tmp);
> +    }
> +
>       return xs_set_permissions(h->xsh, t, path, perms_list,
>                                 ARRAY_SIZE(perms_list));
>   }
> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
> index 90cca85f52..273e414559 100644
> --- a/include/hw/xen/xen_backend_ops.h
> +++ b/include/hw/xen/xen_backend_ops.h
> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>   #define XS_PERM_READ  0x01
>   #define XS_PERM_WRITE 0x02
>   
> +#define XS_PRESERVE_OWNER        0xFFFE
> +
>   struct xenstore_backend_ops {
>       struct qemu_xs_handle *(*open)(void);
>       void (*close)(struct qemu_xs_handle *h);

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

Posted by David Woodhouse 2 years, 3 months ago

On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
> Add option to preserve owner when creating an entry in Xen Store. This
> may be needed in cases when Qemu is working as device model in a
> domain that is Domain-0, e.g. in driver domain.
> 
> "owner" parameter for qemu_xen_xs_create() function can have special
> value XS_PRESERVE_OWNER, which will make specific implementation to
> get original owner of an entry and pass it back to
> set_permissions() call.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

I like this, although I'd like it more if XenStore itself offered this
facility rather than making QEMU do it. Can we make it abundantly clear
that XS_PRESERVE_OWNER is a QEMU internal thing?

>  hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>  hw/xen/xen-operations.c          | 12 ++++++++++++
>  include/hw/xen/xen_backend_ops.h |  2 ++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
> index 660d0b72f9..7b894a9884 100644
> --- a/hw/i386/kvm/xen_xenstore.c
> +++ b/hw/i386/kvm/xen_xenstore.c
> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>          return false;
>      }
>  
> +    if (owner == XS_PRESERVE_OWNER) {
> +        GList *perms;
> +        char letter;
> +
> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
> +        if (err) {
> +            errno = err;
> +            return false;
> +        }

I guess we get away without a race here because it's all internal and
we're holding the QEMU iothread mutex? Perhaps assert that?

> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {

I'd be slightly happier if you used parse_perm() from xenstore_impl.c,
but it's static so I suppose that's fair enough.

> +            errno = EFAULT;
> +            g_list_free_full(perms, g_free);
> +            return false;
> +        }
> +        g_list_free_full(perms, g_free);
> +    }
> +
>      perms_list = g_list_append(perms_list,
>                                 xs_perm_as_string(XS_PERM_NONE, owner));
>      perms_list = g_list_append(perms_list,
> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
> index e00983ec44..1df59b3c08 100644
> --- a/hw/xen/xen-operations.c
> +++ b/hw/xen/xen-operations.c
> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>          return false;
>      }
>  
> +    if (owner == XS_PRESERVE_OWNER) {
> +        struct xs_permissions *tmp;
> +        unsigned int num;
> +
> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> +        if (tmp == NULL) {
> +            return false;
> +        }
> +        perms_list[0].id = tmp[0].id;
> +        free(tmp);
> +    }
> +

Don't see what saves you from someone else changing it at this point on
true Xen though. Which is why I'd prefer XenStore to do it natively.

>      return xs_set_permissions(h->xsh, t, path, perms_list,
>                                ARRAY_SIZE(perms_list));
>  }
> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
> index 90cca85f52..273e414559 100644
> --- a/include/hw/xen/xen_backend_ops.h
> +++ b/include/hw/xen/xen_backend_ops.h
> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>  #define XS_PERM_READ  0x01
>  #define XS_PERM_WRITE 0x02
>  
> +#define XS_PRESERVE_OWNER        0xFFFE
> +
>  struct xenstore_backend_ops {
>      struct qemu_xs_handle *(*open)(void);
>      void (*close)(struct qemu_xs_handle *h);

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

Posted by Volodymyr Babchuk 2 years, 2 months ago

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Fri, 2023-11-10 at 20:42 +0000, Volodymyr Babchuk wrote:
>> Add option to preserve owner when creating an entry in Xen Store. This
>> may be needed in cases when Qemu is working as device model in a
>> domain that is Domain-0, e.g. in driver domain.
>> 
>> "owner" parameter for qemu_xen_xs_create() function can have special
>> value XS_PRESERVE_OWNER, which will make specific implementation to
>> get original owner of an entry and pass it back to
>> set_permissions() call.
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>
> I like this, although I'd like it more if XenStore itself offered this
> facility rather than making QEMU do it.

XenStore itself ensures that access rights are inherited. The problem
is with qemu_xen_xs_create() function that does two things at a time:
creates a new entry and then assigns permissions, overwriting any
permissions that existed before.

> Can we make it abundantly clear
> that XS_PRESERVE_OWNER is a QEMU internal thing?

It is defined in xen_backend_ops.h which is internal QEMU interface for
XenStore. Do you have any suggestions how to make it even clearer?

>>  hw/i386/kvm/xen_xenstore.c       | 18 ++++++++++++++++++
>>  hw/xen/xen-operations.c          | 12 ++++++++++++
>>  include/hw/xen/xen_backend_ops.h |  2 ++
>>  3 files changed, 32 insertions(+)
>> 
>> diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
>> index 660d0b72f9..7b894a9884 100644
>> --- a/hw/i386/kvm/xen_xenstore.c
>> +++ b/hw/i386/kvm/xen_xenstore.c
>> @@ -1572,6 +1572,24 @@ static bool xs_be_create(struct qemu_xs_handle *h, xs_transaction_t t,
>>          return false;
>>      }
>>  
>> +    if (owner == XS_PRESERVE_OWNER) {
>> +        GList *perms;
>> +        char letter;
>> +
>> +        err = xs_impl_get_perms(h->impl, 0, t, path, &perms);
>> +        if (err) {
>> +            errno = err;
>> +            return false;
>> +        }
>
> I guess we get away without a race here because it's all internal and
> we're holding the QEMU iothread mutex? Perhaps assert that?
>

I am not quite familiar with QEMU internals, but why we do we need
assert here? xe_be_create() calls xs_impl* function before and after
this part. Is this piece of code special in some way?


>> +        if (sscanf(perms->data, "%c%u", &letter, &owner) != 2) {
>
> I'd be slightly happier if you used parse_perm() from xenstore_impl.c,
> but it's static so I suppose that's fair enough.
>

Yes, I wanted to use that function, but it is internal for
xenstore_impl.c

I can rename it to xs_impl_parse_perm() and make it public, if you
believe that this is a better approach.

>> +            errno = EFAULT;
>> +            g_list_free_full(perms, g_free);
>> +            return false;
>> +        }
>> +        g_list_free_full(perms, g_free);
>> +    }
>> +
>>      perms_list = g_list_append(perms_list,
>>                                 xs_perm_as_string(XS_PERM_NONE, owner));
>>      perms_list = g_list_append(perms_list,
>> diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
>> index e00983ec44..1df59b3c08 100644
>> --- a/hw/xen/xen-operations.c
>> +++ b/hw/xen/xen-operations.c
>> @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>>          return false;
>>      }
>>  
>> +    if (owner == XS_PRESERVE_OWNER) {
>> +        struct xs_permissions *tmp;
>> +        unsigned int num;
>> +
>> +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
>> +        if (tmp == NULL) {
>> +            return false;
>> +        }
>> +        perms_list[0].id = tmp[0].id;
>> +        free(tmp);
>> +    }
>> +
>
> Don't see what saves you from someone else changing it at this point on
> true Xen though. Which is why I'd prefer XenStore to do it natively.
>

Oh, I missed the transaction parameter. My bad. Will fix in the next
version.

>>      return xs_set_permissions(h->xsh, t, path, perms_list,
>>                                ARRAY_SIZE(perms_list));
>>  }
>> diff --git a/include/hw/xen/xen_backend_ops.h b/include/hw/xen/xen_backend_ops.h
>> index 90cca85f52..273e414559 100644
>> --- a/include/hw/xen/xen_backend_ops.h
>> +++ b/include/hw/xen/xen_backend_ops.h
>> @@ -266,6 +266,8 @@ typedef uint32_t xs_transaction_t;
>>  #define XS_PERM_READ  0x01
>>  #define XS_PERM_WRITE 0x02
>>  
>> +#define XS_PRESERVE_OWNER        0xFFFE
>> +
>>  struct xenstore_backend_ops {
>>      struct qemu_xs_handle *(*open)(void);
>>      void (*close)(struct qemu_xs_handle *h);
>
> [[End of S/MIME Signed Part]]


-- 
WBR, Volodymyr

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

Posted by David Woodhouse 2 years, 3 months ago

On Sat, 2023-11-11 at 11:01 +0000, David Woodhouse wrote:
> 
> > --- a/hw/xen/xen-operations.c
> > +++ b/hw/xen/xen-operations.c
> > @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
> >          return false;
> >      }
> >  
> > +    if (owner == XS_PRESERVE_OWNER) {
> > +        struct xs_permissions *tmp;
> > +        unsigned int num;
> > +
> > +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
> > +        if (tmp == NULL) {
> > +            return false;
> > +        }
> > +        perms_list[0].id = tmp[0].id;
> > +        free(tmp);
> > +    }
> > +
> 
> Don't see what saves you from someone else changing it at this point on
> true Xen though. Which is why I'd prefer XenStore to do it natively.

I suppose maybe you could do it in a transaction *if* the transaction_t
you're passed in isn't already XBT_NULL?

One might argue that the mkdir+set_perms in libxenstore_create() ought
to have been within the same transaction *anyway*?

Re: [PATCH v1 3/7] xen: xenstore: add possibility to preserve owner

Posted by Volodymyr Babchuk 2 years, 2 months ago

Hi David,

David Woodhouse <dwmw2@infradead.org> writes:

> [[S/MIME Signed Part:Undecided]]
> On Sat, 2023-11-11 at 11:01 +0000, David Woodhouse wrote:
>> 
>> > --- a/hw/xen/xen-operations.c
>> > +++ b/hw/xen/xen-operations.c
>> > @@ -300,6 +300,18 @@ static bool libxenstore_create(struct qemu_xs_handle *h, xs_transaction_t t,
>> >          return false;
>> >      }
>> >  
>> > +    if (owner == XS_PRESERVE_OWNER) {
>> > +        struct xs_permissions *tmp;
>> > +        unsigned int num;
>> > +
>> > +        tmp = xs_get_permissions(h->xsh, 0, path, &num);
>> > +        if (tmp == NULL) {
>> > +            return false;
>> > +        }
>> > +        perms_list[0].id = tmp[0].id;
>> > +        free(tmp);
>> > +    }
>> > +
>> 
>> Don't see what saves you from someone else changing it at this point on
>> true Xen though. Which is why I'd prefer XenStore to do it natively.
>
> I suppose maybe you could do it in a transaction *if* the transaction_t
> you're passed in isn't already XBT_NULL?

Yes, I need to pass "t" to xs_get_permissions(), of course.

> One might argue that the mkdir+set_perms in libxenstore_create() ought
> to have been within the same transaction *anyway*? 

Yes, all operations should be performed inside one transaction.

-- 
WBR, Volodymyr

[PATCH v1 1/7] xen-block: Do not write frontend nodes
[PATCH v1 2/7] xen-bus: Do not destroy frontend/backend directories
[PATCH v1 3/7] xen: xenstore: add possibility to preserve owner
[PATCH v1 4/7] xen_pvdev: Do not assume Dom0 when creating a directrory
[PATCH v1 5/7] xen-bus: Set offline if backend's state is XenbusStateClosed
[PATCH v1 6/7] xen_arm: Set mc->max_cpus to GUEST_MAX_VCPUS in xen_arm_init()
[PATCH v1 7/7] xen_arm: Add basic virtio-pci support