[v2,4/6] xen_pvdev: Do not assume Dom0 when creating a directory

Commit Message

Volodymyr Babchuk Nov. 21, 2023, 10:10 p.m. UTC

From: Oleksandr Tyshchenko <[e-mail]>

Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
inherit the owner of the directory.

Note that for other than Dom0 domain (non toolstack domain) the
"driver_domain" property should be set in domain config file for the
toolstack to create required directories in advance.

Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
Signed-off-by: Volodymyr Babchuk <[e-mail]>
---
 hw/xen/xen_pvdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Paul Durrant Nov. 22, 2023, 5:11 p.m. UTC | #1

On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> From: Oleksandr Tyshchenko <[e-mail]>
> 
> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> inherit the owner of the directory.

Ah... so that's why the previous patch is there.

This is not the right way to fix it. The QEMU Xen support is *assuming* 
that QEMU is either running in, or emulating, dom0. In the emulation 
case this is probably fine, but the 'real Xen' case it should be using 
the correct domid for node creation. I guess this could either be 
supplied on the command line or discerned by reading the local domain 
'domid' node.

> 
> Note that for other than Dom0 domain (non toolstack domain) the
> "driver_domain" property should be set in domain config file for the
> toolstack to create required directories in advance.
> 
> Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> Signed-off-by: Volodymyr Babchuk <[e-mail]>
> ---
>   hw/xen/xen_pvdev.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> index c5ad71e8dc..42bdd4f6c8 100644
> --- a/hw/xen/xen_pvdev.c
> +++ b/hw/xen/xen_pvdev.c
> @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
>   
>   int xenstore_mkdir(char *path, int p)
>   {
> -    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
> +    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
> +                            xen_domid, p, path)) {
>           xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
>           return -1;
>       }

Stefano Stabellini Nov. 22, 2023, 10:29 p.m. UTC | #2

On Wed, 22 Nov 2023, Paul Durrant wrote:
> On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > From: Oleksandr Tyshchenko <[e-mail]>
> > 
> > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > inherit the owner of the directory.
> 
> Ah... so that's why the previous patch is there.
> 
> This is not the right way to fix it. The QEMU Xen support is *assuming* that
> QEMU is either running in, or emulating, dom0. In the emulation case this is
> probably fine, but the 'real Xen' case it should be using the correct domid
> for node creation. I guess this could either be supplied on the command line
> or discerned by reading the local domain 'domid' node.

yes, it should be passed as command line option to QEMU

 
> > Note that for other than Dom0 domain (non toolstack domain) the
> > "driver_domain" property should be set in domain config file for the
> > toolstack to create required directories in advance.
> > 
> > Signed-off-by: Oleksandr Tyshchenko <[e-mail]>
> > Signed-off-by: Volodymyr Babchuk <[e-mail]>
> > ---
> >   hw/xen/xen_pvdev.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> > index c5ad71e8dc..42bdd4f6c8 100644
> > --- a/hw/xen/xen_pvdev.c
> > +++ b/hw/xen/xen_pvdev.c
> > @@ -60,7 +60,8 @@ void xen_config_cleanup(void)
> >     int xenstore_mkdir(char *path, int p)
> >   {
> > -    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
> > +    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
> > +                            xen_domid, p, path)) {
> >           xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
> >           return -1;
> >       }
>

David Woodhouse Nov. 22, 2023, 11:03 p.m. UTC | #3

On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
> On Wed, 22 Nov 2023, Paul Durrant wrote:
> > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > > From: Oleksandr Tyshchenko <[e-mail]>
> > > 
> > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > > inherit the owner of the directory.
> > 
> > Ah... so that's why the previous patch is there.
> > 
> > This is not the right way to fix it. The QEMU Xen support is *assuming* that
> > QEMU is either running in, or emulating, dom0. In the emulation case this is
> > probably fine, but the 'real Xen' case it should be using the correct domid
> > for node creation. I guess this could either be supplied on the command line
> > or discerned by reading the local domain 'domid' node.
> 
> yes, it should be passed as command line option to QEMU

I'm not sure I like the idea of a command line option for something
which QEMU could discover for itself.

Stefano Stabellini Nov. 22, 2023, 11:09 p.m. UTC | #4

On Wed, 22 Nov 2023, David Woodhouse wrote:
> On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
> > On Wed, 22 Nov 2023, Paul Durrant wrote:
> > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > > > From: Oleksandr Tyshchenko <[e-mail]>
> > > > 
> > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > > > inherit the owner of the directory.
> > > 
> > > Ah... so that's why the previous patch is there.
> > > 
> > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
> > > QEMU is either running in, or emulating, dom0. In the emulation case this is
> > > probably fine, but the 'real Xen' case it should be using the correct domid
> > > for node creation. I guess this could either be supplied on the command line
> > > or discerned by reading the local domain 'domid' node.
> > 
> > yes, it should be passed as command line option to QEMU
> 
> I'm not sure I like the idea of a command line option for something
> which QEMU could discover for itself.

That's fine too. I meant to say "yes, as far as I know the toolstack
passes the domid to QEMU as a command line option today".

David Woodhouse Nov. 22, 2023, 11:11 p.m. UTC | #5

On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
> On Wed, 22 Nov 2023, David Woodhouse wrote:
> > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
> > > On Wed, 22 Nov 2023, Paul Durrant wrote:
> > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > > > > From: Oleksandr Tyshchenko <[e-mail]>
> > > > > 
> > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > > > > inherit the owner of the directory.
> > > > 
> > > > Ah... so that's why the previous patch is there.
> > > > 
> > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
> > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
> > > > probably fine, but the 'real Xen' case it should be using the correct domid
> > > > for node creation. I guess this could either be supplied on the command line
> > > > or discerned by reading the local domain 'domid' node.
> > > 
> > > yes, it should be passed as command line option to QEMU
> > 
> > I'm not sure I like the idea of a command line option for something
> > which QEMU could discover for itself.
> 
> That's fine too. I meant to say "yes, as far as I know the toolstack
> passes the domid to QEMU as a command line option today".

The -xen-domid argument on the QEMU command line today is the *guest*
domain ID, not the domain ID in which QEMU itself is running.

Or were you thinking of something different?

Stefano Stabellini Nov. 22, 2023, 11:20 p.m. UTC | #6

On Wed, 22 Nov 2023, David Woodhouse wrote:
> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
> > On Wed, 22 Nov 2023, David Woodhouse wrote:
> > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
> > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
> > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > > > > > From: Oleksandr Tyshchenko <[e-mail]>
> > > > > > 
> > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > > > > > inherit the owner of the directory.
> > > > > 
> > > > > Ah... so that's why the previous patch is there.
> > > > > 
> > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
> > > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
> > > > > probably fine, but the 'real Xen' case it should be using the correct domid
> > > > > for node creation. I guess this could either be supplied on the command line
> > > > > or discerned by reading the local domain 'domid' node.
> > > > 
> > > > yes, it should be passed as command line option to QEMU
> > > 
> > > I'm not sure I like the idea of a command line option for something
> > > which QEMU could discover for itself.
> > 
> > That's fine too. I meant to say "yes, as far as I know the toolstack
> > passes the domid to QEMU as a command line option today".
> 
> The -xen-domid argument on the QEMU command line today is the *guest*
> domain ID, not the domain ID in which QEMU itself is running.
> 
> Or were you thinking of something different?

Ops, you are right and I understand your comment better now. The backend
domid is not on the command line but it should be discoverable (on
xenstore if I remember right).

Volodymyr Babchuk Nov. 22, 2023, 11:46 p.m. UTC | #7

Hi Stefano,

Stefano Stabellini <[e-mail]> writes:

> On Wed, 22 Nov 2023, David Woodhouse wrote:
>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>> > On Wed, 22 Nov 2023, David Woodhouse wrote:
>> > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>> > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
>> > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>> > > > > > From: Oleksandr Tyshchenko <[e-mail]>
>> > > > > > 
>> > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>> > > > > > inherit the owner of the directory.
>> > > > > 
>> > > > > Ah... so that's why the previous patch is there.
>> > > > > 
>> > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
>> > > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
>> > > > > probably fine, but the 'real Xen' case it should be using the correct domid
>> > > > > for node creation. I guess this could either be supplied on the command line
>> > > > > or discerned by reading the local domain 'domid' node.
>> > > > 
>> > > > yes, it should be passed as command line option to QEMU
>> > > 
>> > > I'm not sure I like the idea of a command line option for something
>> > > which QEMU could discover for itself.
>> > 
>> > That's fine too. I meant to say "yes, as far as I know the toolstack
>> > passes the domid to QEMU as a command line option today".
>> 
>> The -xen-domid argument on the QEMU command line today is the *guest*
>> domain ID, not the domain ID in which QEMU itself is running.
>> 
>> Or were you thinking of something different?
>
> Ops, you are right and I understand your comment better now. The backend
> domid is not on the command line but it should be discoverable (on
> xenstore if I remember right).

Yes, it is just "~/domid". I'll add a function that reads it.

Volodymyr Babchuk Nov. 23, 2023, 12:07 a.m. UTC | #8

Hi,

Volodymyr Babchuk <[e-mail]> writes:

> Hi Stefano,
>
> Stefano Stabellini <[e-mail]> writes:
>
>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>> > On Wed, 22 Nov 2023, David Woodhouse wrote:
>>> > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>> > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
>>> > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>> > > > > > From: Oleksandr Tyshchenko <[e-mail]>
>>> > > > > > 
>>> > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>>> > > > > > inherit the owner of the directory.
>>> > > > > 
>>> > > > > Ah... so that's why the previous patch is there.
>>> > > > > 
>>> > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
>>> > > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
>>> > > > > probably fine, but the 'real Xen' case it should be using the correct domid
>>> > > > > for node creation. I guess this could either be supplied on the command line
>>> > > > > or discerned by reading the local domain 'domid' node.
>>> > > > 
>>> > > > yes, it should be passed as command line option to QEMU
>>> > > 
>>> > > I'm not sure I like the idea of a command line option for something
>>> > > which QEMU could discover for itself.
>>> > 
>>> > That's fine too. I meant to say "yes, as far as I know the toolstack
>>> > passes the domid to QEMU as a command line option today".
>>> 
>>> The -xen-domid argument on the QEMU command line today is the *guest*
>>> domain ID, not the domain ID in which QEMU itself is running.
>>> 
>>> Or were you thinking of something different?
>>
>> Ops, you are right and I understand your comment better now. The backend
>> domid is not on the command line but it should be discoverable (on
>> xenstore if I remember right).
>
> Yes, it is just "~/domid". I'll add a function that reads it.

Just a quick question to QEMU folks: is it better to add a global
variable where we will store own Domain ID or it will be okay to read
domid from Xenstore every time we need it?

If global variable variant is better, what is proffered place to define
this variable? system/globals.c ?

Paul Durrant Nov. 23, 2023, 9:28 a.m. UTC | #9

On 23/11/2023 00:07, Volodymyr Babchuk wrote:
> 
> Hi,
> 
> Volodymyr Babchuk <[e-mail]> writes:
> 
>> Hi Stefano,
>>
>> Stefano Stabellini <[e-mail]> writes:
>>
>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>>> On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>>>>>> On Wed, 22 Nov 2023, Paul Durrant wrote:
>>>>>>>> On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>>>>>>>> From: Oleksandr Tyshchenko <[e-mail]>
>>>>>>>>>
>>>>>>>>> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>>>>>>>>> inherit the owner of the directory.
>>>>>>>>
>>>>>>>> Ah... so that's why the previous patch is there.
>>>>>>>>
>>>>>>>> This is not the right way to fix it. The QEMU Xen support is *assuming* that
>>>>>>>> QEMU is either running in, or emulating, dom0. In the emulation case this is
>>>>>>>> probably fine, but the 'real Xen' case it should be using the correct domid
>>>>>>>> for node creation. I guess this could either be supplied on the command line
>>>>>>>> or discerned by reading the local domain 'domid' node.
>>>>>>>
>>>>>>> yes, it should be passed as command line option to QEMU
>>>>>>
>>>>>> I'm not sure I like the idea of a command line option for something
>>>>>> which QEMU could discover for itself.
>>>>>
>>>>> That's fine too. I meant to say "yes, as far as I know the toolstack
>>>>> passes the domid to QEMU as a command line option today".
>>>>
>>>> The -xen-domid argument on the QEMU command line today is the *guest*
>>>> domain ID, not the domain ID in which QEMU itself is running.
>>>>
>>>> Or were you thinking of something different?
>>>
>>> Ops, you are right and I understand your comment better now. The backend
>>> domid is not on the command line but it should be discoverable (on
>>> xenstore if I remember right).
>>
>> Yes, it is just "~/domid". I'll add a function that reads it.
> 
> Just a quick question to QEMU folks: is it better to add a global
> variable where we will store own Domain ID or it will be okay to read
> domid from Xenstore every time we need it?
> 
> If global variable variant is better, what is proffered place to define
> this variable? system/globals.c ?
> 

Actually... is it possible for QEMU just to use a relative path for the 
backend nodes? That way it won't need to know it's own domid, will it?

   Paul

David Woodhouse Nov. 23, 2023, 10:45 a.m. UTC | #10

On Thu, 2023-11-23 at 09:28 +0000, Paul Durrant wrote:
> On 23/11/2023 00:07, Volodymyr Babchuk wrote:
> > 
> > Hi,
> > 
> > Volodymyr Babchuk <[e-mail]> writes:
> > 
> > > Hi Stefano,
> > > 
> > > Stefano Stabellini <[e-mail]> writes:
> > > 
> > > > On Wed, 22 Nov 2023, David Woodhouse wrote:
> > > > > On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
> > > > > > On Wed, 22 Nov 2023, David Woodhouse wrote:
> > > > > > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
> > > > > > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
> > > > > > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
> > > > > > > > > > From: Oleksandr Tyshchenko <[e-mail]>
> > > > > > > > > > 
> > > > > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
> > > > > > > > > > inherit the owner of the directory.
> > > > > > > > > 
> > > > > > > > > Ah... so that's why the previous patch is there.
> > > > > > > > > 
> > > > > > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
> > > > > > > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
> > > > > > > > > probably fine, but the 'real Xen' case it should be using the correct domid
> > > > > > > > > for node creation. I guess this could either be supplied on the command line
> > > > > > > > > or discerned by reading the local domain 'domid' node.
> > > > > > > > 
> > > > > > > > yes, it should be passed as command line option to QEMU
> > > > > > > 
> > > > > > > I'm not sure I like the idea of a command line option for something
> > > > > > > which QEMU could discover for itself.
> > > > > > 
> > > > > > That's fine too. I meant to say "yes, as far as I know the toolstack
> > > > > > passes the domid to QEMU as a command line option today".
> > > > > 
> > > > > The -xen-domid argument on the QEMU command line today is the *guest*
> > > > > domain ID, not the domain ID in which QEMU itself is running.
> > > > > 
> > > > > Or were you thinking of something different?
> > > > 
> > > > Ops, you are right and I understand your comment better now. The backend
> > > > domid is not on the command line but it should be discoverable (on
> > > > xenstore if I remember right).
> > > 
> > > Yes, it is just "~/domid". I'll add a function that reads it.
> > 
> > Just a quick question to QEMU folks: is it better to add a global
> > variable where we will store own Domain ID or it will be okay to read
> > domid from Xenstore every time we need it?
> > 
> > If global variable variant is better, what is proffered place to define
> > this variable? system/globals.c ?
> > 
> 
> Actually... is it possible for QEMU just to use a relative path for the 
> backend nodes? That way it won't need to know it's own domid, will it?

That covers some of the use cases, but it may also need to know its own
domid for other purposes. Including writing the *absolute* path of the
backend, to a frontend node?

Volodymyr Babchuk Nov. 23, 2023, 11:43 a.m. UTC | #11

Hi David,

David Woodhouse <[e-mail]> writes:

> [[S/MIME Signed Part:Undecided]]
> On Thu, 2023-11-23 at 09:28 +0000, Paul Durrant wrote:
>> On 23/11/2023 00:07, Volodymyr Babchuk wrote:
>> > 
>> > Hi,
>> > 
>> > Volodymyr Babchuk <[e-mail]> writes:
>> > 
>> > > Hi Stefano,
>> > > 
>> > > Stefano Stabellini <[e-mail]> writes:
>> > > 
>> > > > On Wed, 22 Nov 2023, David Woodhouse wrote:
>> > > > > On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>> > > > > > On Wed, 22 Nov 2023, David Woodhouse wrote:
>> > > > > > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>> > > > > > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
>> > > > > > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>> > > > > > > > > > From: Oleksandr Tyshchenko <[e-mail]>
>> > > > > > > > > > 
>> > > > > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>> > > > > > > > > > inherit the owner of the directory.
>> > > > > > > > > 
>> > > > > > > > > Ah... so that's why the previous patch is there.
>> > > > > > > > > 
>> > > > > > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
>> > > > > > > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
>> > > > > > > > > probably fine, but the 'real Xen' case it should be using the correct domid
>> > > > > > > > > for node creation. I guess this could either be supplied on the command line
>> > > > > > > > > or discerned by reading the local domain 'domid' node.
>> > > > > > > > 
>> > > > > > > > yes, it should be passed as command line option to QEMU
>> > > > > > > 
>> > > > > > > I'm not sure I like the idea of a command line option for something
>> > > > > > > which QEMU could discover for itself.
>> > > > > > 
>> > > > > > That's fine too. I meant to say "yes, as far as I know the toolstack
>> > > > > > passes the domid to QEMU as a command line option today".
>> > > > > 
>> > > > > The -xen-domid argument on the QEMU command line today is the *guest*
>> > > > > domain ID, not the domain ID in which QEMU itself is running.
>> > > > > 
>> > > > > Or were you thinking of something different?
>> > > > 
>> > > > Ops, you are right and I understand your comment better now. The backend
>> > > > domid is not on the command line but it should be discoverable (on
>> > > > xenstore if I remember right).
>> > > 
>> > > Yes, it is just "~/domid". I'll add a function that reads it.
>> > 
>> > Just a quick question to QEMU folks: is it better to add a global
>> > variable where we will store own Domain ID or it will be okay to read
>> > domid from Xenstore every time we need it?
>> > 
>> > If global variable variant is better, what is proffered place to define
>> > this variable? system/globals.c ?
>> > 
>> 
>> Actually... is it possible for QEMU just to use a relative path for the 
>> backend nodes? That way it won't need to know it's own domid, will it?
>
> That covers some of the use cases, but it may also need to know its own
> domid for other purposes. Including writing the *absolute* path of the
> backend, to a frontend node?

Is this case possible? As I understand, QEMU writes frontend nodes only
when it emulates Xen, otherwise this done by Xen toolstack. And in case
of Xen emulation, QEMU always assumes role of Domain-0.

David Woodhouse Nov. 23, 2023, 11:51 a.m. UTC | #12

On 23 November 2023 11:43:35 GMT, Volodymyr Babchuk <[e-mail]> wrote:
>
>Hi David,
>
>David Woodhouse <[e-mail]> writes:
>
>> [[S/MIME Signed Part:Undecided]]
>> On Thu, 2023-11-23 at 09:28 +0000, Paul Durrant wrote:
>>> On 23/11/2023 00:07, Volodymyr Babchuk wrote:
>>> > 
>>> > Hi,
>>> > 
>>> > Volodymyr Babchuk <[e-mail]> writes:
>>> > 
>>> > > Hi Stefano,
>>> > > 
>>> > > Stefano Stabellini <[e-mail]> writes:
>>> > > 
>>> > > > On Wed, 22 Nov 2023, David Woodhouse wrote:
>>> > > > > On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>> > > > > > On Wed, 22 Nov 2023, David Woodhouse wrote:
>>> > > > > > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>> > > > > > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
>>> > > > > > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>> > > > > > > > > > From: Oleksandr Tyshchenko <[e-mail]>
>>> > > > > > > > > > 
>>> > > > > > > > > > Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>>> > > > > > > > > > inherit the owner of the directory.
>>> > > > > > > > > 
>>> > > > > > > > > Ah... so that's why the previous patch is there.
>>> > > > > > > > > 
>>> > > > > > > > > This is not the right way to fix it. The QEMU Xen support is *assuming* that
>>> > > > > > > > > QEMU is either running in, or emulating, dom0. In the emulation case this is
>>> > > > > > > > > probably fine, but the 'real Xen' case it should be using the correct domid
>>> > > > > > > > > for node creation. I guess this could either be supplied on the command line
>>> > > > > > > > > or discerned by reading the local domain 'domid' node.
>>> > > > > > > > 
>>> > > > > > > > yes, it should be passed as command line option to QEMU
>>> > > > > > > 
>>> > > > > > > I'm not sure I like the idea of a command line option for something
>>> > > > > > > which QEMU could discover for itself.
>>> > > > > > 
>>> > > > > > That's fine too. I meant to say "yes, as far as I know the toolstack
>>> > > > > > passes the domid to QEMU as a command line option today".
>>> > > > > 
>>> > > > > The -xen-domid argument on the QEMU command line today is the *guest*
>>> > > > > domain ID, not the domain ID in which QEMU itself is running.
>>> > > > > 
>>> > > > > Or were you thinking of something different?
>>> > > > 
>>> > > > Ops, you are right and I understand your comment better now. The backend
>>> > > > domid is not on the command line but it should be discoverable (on
>>> > > > xenstore if I remember right).
>>> > > 
>>> > > Yes, it is just "~/domid". I'll add a function that reads it.
>>> > 
>>> > Just a quick question to QEMU folks: is it better to add a global
>>> > variable where we will store own Domain ID or it will be okay to read
>>> > domid from Xenstore every time we need it?
>>> > 
>>> > If global variable variant is better, what is proffered place to define
>>> > this variable? system/globals.c ?
>>> > 
>>> 
>>> Actually... is it possible for QEMU just to use a relative path for the 
>>> backend nodes? That way it won't need to know it's own domid, will it?
>>
>> That covers some of the use cases, but it may also need to know its own
>> domid for other purposes. Including writing the *absolute* path of the
>> backend, to a frontend node?
>
>Is this case possible? As I understand, QEMU writes frontend nodes only
>when it emulates Xen, otherwise this done by Xen toolstack. And in case
>of Xen emulation, QEMU always assumes role of Domain-0.


No, you can hotplug and unplug devices in QEMU even under real Xen. And if QEMU in a driver domain is given sufficient permission to write to its guest's frontend nodes, it should not need to be in dom0.

Volodymyr Babchuk Nov. 23, 2023, 11:54 a.m. UTC | #13

Hi Paul,

Paul Durrant <[e-mail]> writes:

> On 23/11/2023 00:07, Volodymyr Babchuk wrote:
>> Hi,
>> Volodymyr Babchuk <[e-mail]> writes:
>> 
>>> Hi Stefano,
>>>
>>> Stefano Stabellini <[e-mail]> writes:
>>>
>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>>>> On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>>>>>>> On Wed, 22 Nov 2023, Paul Durrant wrote:
>>>>>>>>> On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>>>>>>>>> From: Oleksandr Tyshchenko <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>>>>>>>>>> inherit the owner of the directory.
>>>>>>>>>
>>>>>>>>> Ah... so that's why the previous patch is there.
>>>>>>>>>
>>>>>>>>> This is not the right way to fix it. The QEMU Xen support is *assuming* that
>>>>>>>>> QEMU is either running in, or emulating, dom0. In the emulation case this is
>>>>>>>>> probably fine, but the 'real Xen' case it should be using the correct domid
>>>>>>>>> for node creation. I guess this could either be supplied on the command line
>>>>>>>>> or discerned by reading the local domain 'domid' node.
>>>>>>>>
>>>>>>>> yes, it should be passed as command line option to QEMU
>>>>>>>
>>>>>>> I'm not sure I like the idea of a command line option for something
>>>>>>> which QEMU could discover for itself.
>>>>>>
>>>>>> That's fine too. I meant to say "yes, as far as I know the toolstack
>>>>>> passes the domid to QEMU as a command line option today".
>>>>>
>>>>> The -xen-domid argument on the QEMU command line today is the *guest*
>>>>> domain ID, not the domain ID in which QEMU itself is running.
>>>>>
>>>>> Or were you thinking of something different?
>>>>
>>>> Ops, you are right and I understand your comment better now. The backend
>>>> domid is not on the command line but it should be discoverable (on
>>>> xenstore if I remember right).
>>>
>>> Yes, it is just "~/domid". I'll add a function that reads it.
>> Just a quick question to QEMU folks: is it better to add a global
>> variable where we will store own Domain ID or it will be okay to read
>> domid from Xenstore every time we need it?
>> If global variable variant is better, what is proffered place to
>> define
>> this variable? system/globals.c ?
>> 
>
> Actually... is it possible for QEMU just to use a relative path for
> the backend nodes? That way it won't need to know it's own domid, will
> it?

Well, it is possible to use relative path, AFAIK, linux-based backends
are doing exactly this. But problem is with xenstore_mkdir() function,
which requires domain id to correctly set owner when it causes call to
set_permissions().

As David said, architecturally it will be better to get rid of
xenstore_mkdir() usage, because it is used by legacy backends only. But
to do this, someone needs to convert legacy backends to use newer API. I
have no capacity to do this right now, so I implemented a contained
solution:

static int xenstore_read_own_domid(unsigned int *domid)

in xen_pvdev.c. I believe, this new function will be removed along with
whole xen_pvdev.c when there will be no legacy backends left.

David Woodhouse Nov. 23, 2023, 11:57 a.m. UTC | #14

On 23 November 2023 11:54:01 GMT, Volodymyr Babchuk <[e-mail]> wrote:
>
>Hi Paul,
>
>Paul Durrant <[e-mail]> writes:
>
>> On 23/11/2023 00:07, Volodymyr Babchuk wrote:
>>> Hi,
>>> Volodymyr Babchuk <[e-mail]> writes:
>>> 
>>>> Hi Stefano,
>>>>
>>>> Stefano Stabellini <[e-mail]> writes:
>>>>
>>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>>>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>>>>> On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>>>>>>>> On Wed, 22 Nov 2023, Paul Durrant wrote:
>>>>>>>>>> On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>>>>>>>>>> From: Oleksandr Tyshchenko <[e-mail]>
>>>>>>>>>>>
>>>>>>>>>>> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>>>>>>>>>>> inherit the owner of the directory.
>>>>>>>>>>
>>>>>>>>>> Ah... so that's why the previous patch is there.
>>>>>>>>>>
>>>>>>>>>> This is not the right way to fix it. The QEMU Xen support is *assuming* that
>>>>>>>>>> QEMU is either running in, or emulating, dom0. In the emulation case this is
>>>>>>>>>> probably fine, but the 'real Xen' case it should be using the correct domid
>>>>>>>>>> for node creation. I guess this could either be supplied on the command line
>>>>>>>>>> or discerned by reading the local domain 'domid' node.
>>>>>>>>>
>>>>>>>>> yes, it should be passed as command line option to QEMU
>>>>>>>>
>>>>>>>> I'm not sure I like the idea of a command line option for something
>>>>>>>> which QEMU could discover for itself.
>>>>>>>
>>>>>>> That's fine too. I meant to say "yes, as far as I know the toolstack
>>>>>>> passes the domid to QEMU as a command line option today".
>>>>>>
>>>>>> The -xen-domid argument on the QEMU command line today is the *guest*
>>>>>> domain ID, not the domain ID in which QEMU itself is running.
>>>>>>
>>>>>> Or were you thinking of something different?
>>>>>
>>>>> Ops, you are right and I understand your comment better now. The backend
>>>>> domid is not on the command line but it should be discoverable (on
>>>>> xenstore if I remember right).
>>>>
>>>> Yes, it is just "~/domid". I'll add a function that reads it.
>>> Just a quick question to QEMU folks: is it better to add a global
>>> variable where we will store own Domain ID or it will be okay to read
>>> domid from Xenstore every time we need it?
>>> If global variable variant is better, what is proffered place to
>>> define
>>> this variable? system/globals.c ?
>>> 
>>
>> Actually... is it possible for QEMU just to use a relative path for
>> the backend nodes? That way it won't need to know it's own domid, will
>> it?
>
>Well, it is possible to use relative path, AFAIK, linux-based backends
>are doing exactly this. But problem is with xenstore_mkdir() function,
>which requires domain id to correctly set owner when it causes call to
>set_permissions().
>
>As David said, architecturally it will be better to get rid of
>xenstore_mkdir() usage, because it is used by legacy backends only. But
>to do this, someone needs to convert legacy backends to use newer API. I
>have no capacity to do this right now, so I implemented a contained
>solution:
>
>static int xenstore_read_own_domid(unsigned int *domid)
>
>in xen_pvdev.c. I believe, this new function will be removed along with
>whole xen_pvdev.c when there will be no legacy backends left.

Which PV backends do you care about? We already have net, block and console converted.

Volodymyr Babchuk Nov. 23, 2023, 12:17 p.m. UTC | #15

Hi David,

David Woodhouse <[e-mail]> writes:

> On 23 November 2023 11:54:01 GMT, Volodymyr Babchuk <[e-mail]> wrote:
>>
>>Hi Paul,
>>
>>Paul Durrant <[e-mail]> writes:
>>
>>> On 23/11/2023 00:07, Volodymyr Babchuk wrote:
>>>> Hi,
>>>> Volodymyr Babchuk <[e-mail]> writes:
>>>> 
>>>>> Hi Stefano,
>>>>>
>>>>> Stefano Stabellini <[e-mail]> writes:
>>>>>
>>>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>>>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>>>>>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>>>>>>> On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>>>>>>>>> On Wed, 22 Nov 2023, Paul Durrant wrote:
>>>>>>>>>>> On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>>>>>>>>>>> From: Oleksandr Tyshchenko <[e-mail]>
>>>>>>>>>>>>
>>>>>>>>>>>> Instead of forcing the owner to domid 0, use XS_PRESERVE_OWNER to
>>>>>>>>>>>> inherit the owner of the directory.
>>>>>>>>>>>
>>>>>>>>>>> Ah... so that's why the previous patch is there.
>>>>>>>>>>>
>>>>>>>>>>> This is not the right way to fix it. The QEMU Xen support is *assuming* that
>>>>>>>>>>> QEMU is either running in, or emulating, dom0. In the emulation case this is
>>>>>>>>>>> probably fine, but the 'real Xen' case it should be using the correct domid
>>>>>>>>>>> for node creation. I guess this could either be supplied on the command line
>>>>>>>>>>> or discerned by reading the local domain 'domid' node.
>>>>>>>>>>
>>>>>>>>>> yes, it should be passed as command line option to QEMU
>>>>>>>>>
>>>>>>>>> I'm not sure I like the idea of a command line option for something
>>>>>>>>> which QEMU could discover for itself.
>>>>>>>>
>>>>>>>> That's fine too. I meant to say "yes, as far as I know the toolstack
>>>>>>>> passes the domid to QEMU as a command line option today".
>>>>>>>
>>>>>>> The -xen-domid argument on the QEMU command line today is the *guest*
>>>>>>> domain ID, not the domain ID in which QEMU itself is running.
>>>>>>>
>>>>>>> Or were you thinking of something different?
>>>>>>
>>>>>> Ops, you are right and I understand your comment better now. The backend
>>>>>> domid is not on the command line but it should be discoverable (on
>>>>>> xenstore if I remember right).
>>>>>
>>>>> Yes, it is just "~/domid". I'll add a function that reads it.
>>>> Just a quick question to QEMU folks: is it better to add a global
>>>> variable where we will store own Domain ID or it will be okay to read
>>>> domid from Xenstore every time we need it?
>>>> If global variable variant is better, what is proffered place to
>>>> define
>>>> this variable? system/globals.c ?
>>>> 
>>>
>>> Actually... is it possible for QEMU just to use a relative path for
>>> the backend nodes? That way it won't need to know it's own domid, will
>>> it?
>>
>>Well, it is possible to use relative path, AFAIK, linux-based backends
>>are doing exactly this. But problem is with xenstore_mkdir() function,
>>which requires domain id to correctly set owner when it causes call to
>>set_permissions().
>>
>>As David said, architecturally it will be better to get rid of
>>xenstore_mkdir() usage, because it is used by legacy backends only. But
>>to do this, someone needs to convert legacy backends to use newer API. I
>>have no capacity to do this right now, so I implemented a contained
>>solution:
>>
>>static int xenstore_read_own_domid(unsigned int *domid)
>>
>>in xen_pvdev.c. I believe, this new function will be removed along with
>>whole xen_pvdev.c when there will be no legacy backends left.
>
> Which PV backends do you care about? We already have net, block and console converted.

Well, this is all what we need, actually. Even console only will be
sufficient, as we are using QEMU to provide virtio-pci backends, so both
storage and networking should be provided by virtio. Are you proposing
to just drop this patch at all? I believe we can live without it, yes.

David Woodhouse Nov. 23, 2023, 12:27 p.m. UTC | #16

On 23 November 2023 12:17:57 GMT, Volodymyr Babchuk <[e-mail]> wrote:
>
>Hi David,
>
>David Woodhouse <[e-mail]> writes:
>> Which PV backends do you care about? We already have net, block and console converted.
>
>Well, this is all what we need, actually. Even console only will be
>sufficient, as we are using QEMU to provide virtio-pci backends, so both
>storage and networking should be provided by virtio. Are you proposing
>to just drop this patch at all? I believe we can live without it, yes.

Yeah, I think you can drop anything that's only needed for the legacy backends. I'm tempted to make a separate config option to compile those out.

Paul Durrant Nov. 23, 2023, 12:54 p.m. UTC | #17

On 23/11/2023 12:27, David Woodhouse wrote:
> On 23 November 2023 12:17:57 GMT, Volodymyr Babchuk <[e-mail]> wrote:
>>
>> Hi David,
>>
>> David Woodhouse <[e-mail]> writes:
>>> Which PV backends do you care about? We already have net, block and console converted.
>>
>> Well, this is all what we need, actually. Even console only will be
>> sufficient, as we are using QEMU to provide virtio-pci backends, so both
>> storage and networking should be provided by virtio. Are you proposing
>> to just drop this patch at all? I believe we can live without it, yes.
> 
> Yeah, I think you can drop anything that's only needed for the legacy backends. I'm tempted to make a separate config option to compile those out.
> 

I think that would be a good idea. The other legacy bacckend that we may 
need to care about is xenfb... not so much the framebuffer itself, but 
the mouse and keyboard aspects. The XENVKBD and XENHID drivers expose PV 
mouse and keyboard to Windows instances so it's be nice if we can avoid 
the backend withering away.

   Paul

Volodymyr Babchuk Nov. 24, 2023, 12:24 a.m. UTC | #18

Hi David,

David Woodhouse <[e-mail]> writes:

> On 23 November 2023 12:17:57 GMT, Volodymyr Babchuk <[e-mail]> wrote:
>>
>>Hi David,
>>
>>David Woodhouse <[e-mail]> writes:
>>> Which PV backends do you care about? We already have net, block and console converted.
>>
>>Well, this is all what we need, actually. Even console only will be
>>sufficient, as we are using QEMU to provide virtio-pci backends, so both
>>storage and networking should be provided by virtio. Are you proposing
>>to just drop this patch at all? I believe we can live without it, yes.
>
> Yeah, I think you can drop anything that's only needed for the legacy backends. I'm tempted to make a separate config option to compile those out.

Yep, we need this. Because without the patch ("xen_pvdev: Do not assume
Dom0 when creating a directory") I can't run QEMU in the driver domain:

root@spider-domd:~# qemu-system-aarch64  -machine xenpv -m 128M
xen be core: xs_mkdir device-model/0/backends/vkbd: failed
xen be core: xs_mkdir device-model/0/backends/vkbd: failed
xen be core: xs_mkdir device-model/0/backends/9pfs: failed
xen be core: xs_mkdir device-model/0/backends/9pfs: failed

So yeah, we need something like CONFIG_XEN_LEGACY_BACKENDS option if we
don't want to fix xenstore_mkdir()

Alex Bennée Nov. 24, 2023, 12:56 p.m. UTC | #19

Volodymyr Babchuk <[e-mail]> writes:

> Hi,
>
> Volodymyr Babchuk <[e-mail]> writes:
>
>> Hi Stefano,
>>
>> Stefano Stabellini <[e-mail]> writes:
>>
>>> On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>> On Wed, 2023-11-22 at 15:09 -0800, Stefano Stabellini wrote:
>>>> > On Wed, 22 Nov 2023, David Woodhouse wrote:
>>>> > > On Wed, 2023-11-22 at 14:29 -0800, Stefano Stabellini wrote:
>>>> > > > On Wed, 22 Nov 2023, Paul Durrant wrote:
>>>> > > > > On 21/11/2023 22:10, Volodymyr Babchuk wrote:
>>>> > > > > > From: Oleksandr Tyshchenko <[e-mail]>
<snip>
>>>> The -xen-domid argument on the QEMU command line today is the *guest*
>>>> domain ID, not the domain ID in which QEMU itself is running.
>>>> 
>>>> Or were you thinking of something different?
>>>
>>> Ops, you are right and I understand your comment better now. The backend
>>> domid is not on the command line but it should be discoverable (on
>>> xenstore if I remember right).
>>
>> Yes, it is just "~/domid". I'll add a function that reads it.
>
> Just a quick question to QEMU folks: is it better to add a global
> variable where we will store own Domain ID or it will be okay to read
> domid from Xenstore every time we need it?
>
> If global variable variant is better, what is proffered place to define
> this variable? system/globals.c ?

Hmm maybe, I see Xen already has some but the comment:

  "Global variables that (mostly) should not exist"

I think it to dissuade the file growing more than it should.

I think generally the best pattern to use if a global can't be avoided
is to have a "static global" in the main .c file for the module and then
provide a helper access function for other files to read it. That also
makes re-factoring easier if things like locking need to be added down
the line.

We still do have a few true global variables which need "extern"
declarations in the headers but if we can avoid adding more that would
be good.

Of course ideally this sort of data would be wrapped up in QOM
structures but I can see the argument for the host domain ID.

Patch

diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index c5ad71e8dc..42bdd4f6c8 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -60,7 +60,8 @@  void xen_config_cleanup(void)
 
 int xenstore_mkdir(char *path, int p)
 {
-    if (!qemu_xen_xs_create(xenstore, 0, 0, xen_domid, p, path)) {
+    if (!qemu_xen_xs_create(xenstore, 0, XS_PRESERVE_OWNER,
+                            xen_domid, p, path)) {
         xen_pv_printf(NULL, 0, "xs_mkdir %s: failed\n", path);
         return -1;
     }