diff mbox series

[v4,1/1] virt: acrn: Mark the uuid field as unused

Message ID 20221116092254.66234-1-andriy.shevchenko@linux.intel.com
State New
Headers
Series [v4,1/1] virt: acrn: Mark the uuid field as unused |

Commit Message

Andy Shevchenko Nov. 16, 2022, 9:22 a.m. UTC 
  After the commits for userspace (see Link tags below) the uuid field is
not being used in the ACRN code. Update kernel to reflect these changes.
I.e. we do the following:
- adding a comment explaining that it's not used anymore
- replacing the specific type by a raw buffer
- updating the example code accordingly

The advertised field confused users and actually never been used. So
the wrong part here is that kernel puts something which userspace never
used and hence this may confuse a reader of this code.

Link: https://github.com/projectacrn/acrn-hypervisor/commit/da0d24326ed6
Link: https://github.com/projectacrn/acrn-hypervisor/commit/bb0327e70097
Fixes: 5b06931d7f8b ("sample/acrn: Introduce a sample of HSM ioctl interface usage")
Fixes: 9c5137aedd11 ("virt: acrn: Introduce VM management interfaces")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
---
v4: added tag (Rafael), Cc'ed to Greg (missing in the previous version
    by some reason)
v3: converted to Link tags (Rafael), explained what was wrong (Rafael)
v2: added tag (Fei)

 include/uapi/linux/acrn.h | 5 ++---
 samples/acrn/vm-sample.c  | 3 ---
 2 files changed, 2 insertions(+), 6 deletions(-)

Comments

Greg KH Nov. 16, 2022, 11:42 a.m. UTC | #1 
On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:
> After the commits for userspace (see Link tags below) the uuid field is
> not being used in the ACRN code. Update kernel to reflect these changes.
> I.e. we do the following:
> - adding a comment explaining that it's not used anymore
> - replacing the specific type by a raw buffer
> - updating the example code accordingly
> 
> The advertised field confused users and actually never been used. So
> the wrong part here is that kernel puts something which userspace never
> used and hence this may confuse a reader of this code.
> 
> Link: https://github.com/projectacrn/acrn-hypervisor/commit/da0d24326ed6
> Link: https://github.com/projectacrn/acrn-hypervisor/commit/bb0327e70097
> Fixes: 5b06931d7f8b ("sample/acrn: Introduce a sample of HSM ioctl interface usage")
> Fixes: 9c5137aedd11 ("virt: acrn: Introduce VM management interfaces")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> ---
> v4: added tag (Rafael), Cc'ed to Greg (missing in the previous version
>     by some reason)
> v3: converted to Link tags (Rafael), explained what was wrong (Rafael)
> v2: added tag (Fei)
> 
>  include/uapi/linux/acrn.h | 5 ++---
>  samples/acrn/vm-sample.c  | 3 ---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/uapi/linux/acrn.h b/include/uapi/linux/acrn.h
> index ccf47ed92500..04fa83647ae5 100644
> --- a/include/uapi/linux/acrn.h
> +++ b/include/uapi/linux/acrn.h
> @@ -12,7 +12,6 @@
>  #define _UAPI_ACRN_H
>  
>  #include <linux/types.h>
> -#include <linux/uuid.h>
>  
>  #define ACRN_IO_REQUEST_MAX		16
>  
> @@ -186,7 +185,7 @@ struct acrn_ioreq_notify {
>   * @reserved0:		Reserved and must be 0
>   * @vcpu_num:		Number of vCPU in the VM. Return from hypervisor.
>   * @reserved1:		Reserved and must be 0
> - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> + * @uuid:		Reserved (used to be UUID of the VM)

If it's reserved, then don't you need to check for 0?

>   * @vm_flag:		Flag of the VM creating. Pass to hypervisor directly.
>   * @ioreq_buf:		Service VM GPA of I/O request buffer. Pass to
>   *			hypervisor directly.
> @@ -198,7 +197,7 @@ struct acrn_vm_creation {
>  	__u16	reserved0;
>  	__u16	vcpu_num;
>  	__u16	reserved1;
> -	guid_t	uuid;
> +	__u8	uuid[16];

You just changed the type here, so what is that going to break in
userspace that depended on this being of a structure type and now it's
an array?

And no other kernel changes needed?  Shouldn't you warn if this field is
set?

thanks,

greg k-h
Andy Shevchenko Nov. 16, 2022, 1:29 p.m. UTC | #2 
On Wed, Nov 16, 2022 at 12:42:16PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:
> > After the commits for userspace (see Link tags below) the uuid field is
> > not being used in the ACRN code. Update kernel to reflect these changes.
> > I.e. we do the following:
> > - adding a comment explaining that it's not used anymore
> > - replacing the specific type by a raw buffer
> > - updating the example code accordingly
> > 
> > The advertised field confused users and actually never been used. So
> > the wrong part here is that kernel puts something which userspace never
> > used and hence this may confuse a reader of this code.

> > - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> > + * @uuid:		Reserved (used to be UUID of the VM)
> 
> If it's reserved, then don't you need to check for 0?

Reserved in a way that it may content something we just don't care about.

...

> > +	__u8	uuid[16];
> 
> You just changed the type here, so what is that going to break in
> userspace that depended on this being of a structure type and now it's
> an array?

It's the same. The previous was hidden behind additional type level.

> And no other kernel changes needed?  Shouldn't you warn if this field is
> set?

No, as pointed out in the commit message kernel never ever used this.
Greg KH Nov. 16, 2022, 3:20 p.m. UTC | #3 
On Wed, Nov 16, 2022 at 03:29:31PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 16, 2022 at 12:42:16PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:
> > > After the commits for userspace (see Link tags below) the uuid field is
> > > not being used in the ACRN code. Update kernel to reflect these changes.
> > > I.e. we do the following:
> > > - adding a comment explaining that it's not used anymore
> > > - replacing the specific type by a raw buffer
> > > - updating the example code accordingly
> > > 
> > > The advertised field confused users and actually never been used. So
> > > the wrong part here is that kernel puts something which userspace never
> > > used and hence this may confuse a reader of this code.
> 
> > > - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> > > + * @uuid:		Reserved (used to be UUID of the VM)
> > 
> > If it's reserved, then don't you need to check for 0?
> 
> Reserved in a way that it may content something we just don't care about.

"reserved" in the kernel ioctls mean "must be 0 and we will test for it,
otherwise this is an empty/useless field that can never be touched again
in the future.

Please spell it out in detail as to if you can ever use this later on,
and what the kernel will do (if anything) if it is set.

And if "the kernel ignores it" then that means these bytes are now
"empty space never to be used again", right?

> ...
> 
> > > +	__u8	uuid[16];
> > 
> > You just changed the type here, so what is that going to break in
> > userspace that depended on this being of a structure type and now it's
> > an array?
> 
> It's the same. The previous was hidden behind additional type level.

Same size, yes.  Same C structure definition, no.

> > And no other kernel changes needed?  Shouldn't you warn if this field is
> > set?
> 
> No, as pointed out in the commit message kernel never ever used this.

That does not mean that userspace tools never did, right?  You are
changing the structure definition, what tool just broke?

thanks,

greg k-h
Andy Shevchenko Nov. 16, 2022, 4:04 p.m. UTC | #4 
On Wed, Nov 16, 2022 at 04:20:08PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 16, 2022 at 03:29:31PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 16, 2022 at 12:42:16PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:

...

> > > > - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> > > > + * @uuid:		Reserved (used to be UUID of the VM)
> > > 
> > > If it's reserved, then don't you need to check for 0?
> > 
> > Reserved in a way that it may content something we just don't care about.
> 
> "reserved" in the kernel ioctls mean "must be 0 and we will test for it,
> otherwise this is an empty/useless field that can never be touched again
> in the future.
> 
> Please spell it out in detail as to if you can ever use this later on,
> and what the kernel will do (if anything) if it is set.
> 
> And if "the kernel ignores it" then that means these bytes are now
> "empty space never to be used again", right?

Right, I will fix this in v5.

...

> > > > +	__u8	uuid[16];
> > > 
> > > You just changed the type here, so what is that going to break in
> > > userspace that depended on this being of a structure type and now it's
> > > an array?
> > 
> > It's the same. The previous was hidden behind additional type level.
> 
> Same size, yes.  Same C structure definition, no.

It doesn't matter, see below.

> > > And no other kernel changes needed?  Shouldn't you warn if this field is
> > > set?
> > 
> > No, as pointed out in the commit message kernel never ever used this.
> 
> That does not mean that userspace tools never did, right?  You are
> changing the structure definition, what tool just broke?

The only tool has been amended like a year ago, so the answer is none.
The commit message has links to the commits in question that made that
amendment.

Maybe I should remove Fixes tags? In such case we will very much know
that no old tools will be run on the new kernel.
Greg KH Nov. 16, 2022, 4:16 p.m. UTC | #5 
On Wed, Nov 16, 2022 at 06:04:37PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 16, 2022 at 04:20:08PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 16, 2022 at 03:29:31PM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 16, 2022 at 12:42:16PM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> > > > > + * @uuid:		Reserved (used to be UUID of the VM)
> > > > 
> > > > If it's reserved, then don't you need to check for 0?
> > > 
> > > Reserved in a way that it may content something we just don't care about.
> > 
> > "reserved" in the kernel ioctls mean "must be 0 and we will test for it,
> > otherwise this is an empty/useless field that can never be touched again
> > in the future.
> > 
> > Please spell it out in detail as to if you can ever use this later on,
> > and what the kernel will do (if anything) if it is set.
> > 
> > And if "the kernel ignores it" then that means these bytes are now
> > "empty space never to be used again", right?
> 
> Right, I will fix this in v5.
> 
> ...
> 
> > > > > +	__u8	uuid[16];
> > > > 
> > > > You just changed the type here, so what is that going to break in
> > > > userspace that depended on this being of a structure type and now it's
> > > > an array?
> > > 
> > > It's the same. The previous was hidden behind additional type level.
> > 
> > Same size, yes.  Same C structure definition, no.
> 
> It doesn't matter, see below.
> 
> > > > And no other kernel changes needed?  Shouldn't you warn if this field is
> > > > set?
> > > 
> > > No, as pointed out in the commit message kernel never ever used this.
> > 
> > That does not mean that userspace tools never did, right?  You are
> > changing the structure definition, what tool just broke?
> 
> The only tool has been amended like a year ago, so the answer is none.
> The commit message has links to the commits in question that made that
> amendment.
> 
> Maybe I should remove Fixes tags? In such case we will very much know
> that no old tools will be run on the new kernel.

Please remove "fixes" as this doesn't "fix" anything.

thanks,

greg k-h
Andy Shevchenko Nov. 16, 2022, 4:31 p.m. UTC | #6 
On Wed, Nov 16, 2022 at 05:16:20PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 16, 2022 at 06:04:37PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 16, 2022 at 04:20:08PM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 16, 2022 at 03:29:31PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 16, 2022 at 12:42:16PM +0100, Greg Kroah-Hartman wrote:
> > > > > On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > > > > - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> > > > > > + * @uuid:		Reserved (used to be UUID of the VM)
> > > > > 
> > > > > If it's reserved, then don't you need to check for 0?
> > > > 
> > > > Reserved in a way that it may content something we just don't care about.
> > > 
> > > "reserved" in the kernel ioctls mean "must be 0 and we will test for it,
> > > otherwise this is an empty/useless field that can never be touched again
> > > in the future.
> > > 
> > > Please spell it out in detail as to if you can ever use this later on,
> > > and what the kernel will do (if anything) if it is set.
> > > 
> > > And if "the kernel ignores it" then that means these bytes are now
> > > "empty space never to be used again", right?
> > 
> > Right, I will fix this in v5.
> > 
> > ...
> > 
> > > > > > +	__u8	uuid[16];
> > > > > 
> > > > > You just changed the type here, so what is that going to break in
> > > > > userspace that depended on this being of a structure type and now it's
> > > > > an array?
> > > > 
> > > > It's the same. The previous was hidden behind additional type level.
> > > 
> > > Same size, yes.  Same C structure definition, no.
> > 
> > It doesn't matter, see below.
> > 
> > > > > And no other kernel changes needed?  Shouldn't you warn if this field is
> > > > > set?
> > > > 
> > > > No, as pointed out in the commit message kernel never ever used this.
> > > 
> > > That does not mean that userspace tools never did, right?  You are
> > > changing the structure definition, what tool just broke?
> > 
> > The only tool has been amended like a year ago, so the answer is none.
> > The commit message has links to the commits in question that made that
> > amendment.
> > 
> > Maybe I should remove Fixes tags? In such case we will very much know
> > that no old tools will be run on the new kernel.
> 
> Please remove "fixes" as this doesn't "fix" anything.

Done in v5.

Thank you for the thorough review!
Fei Li Nov. 17, 2022, 1:03 a.m. UTC | #7 
On 2022-11-16 at 18:04:37 +0200, Andy Shevchenko wrote:
> On Wed, Nov 16, 2022 at 04:20:08PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 16, 2022 at 03:29:31PM +0200, Andy Shevchenko wrote:
> > > On Wed, Nov 16, 2022 at 12:42:16PM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Nov 16, 2022 at 11:22:54AM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > - * @uuid:		UUID of the VM. Pass to hypervisor directly.
> > > > > + * @uuid:		Reserved (used to be UUID of the VM)
> > > > 
> > > > If it's reserved, then don't you need to check for 0?
> > > 
> > > Reserved in a way that it may content something we just don't care about.
> > 
> > "reserved" in the kernel ioctls mean "must be 0 and we will test for it,
> > otherwise this is an empty/useless field that can never be touched again
> > in the future.
> > 
> > Please spell it out in detail as to if you can ever use this later on,
> > and what the kernel will do (if anything) if it is set.
> > 
> > And if "the kernel ignores it" then that means these bytes are now
> > "empty space never to be used again", right?
> 
> Right, I will fix this in v5.
Hi Andy

ACRN does not use uuid recently. But before that, the old ACRN still uses it.
So could we just change the uuid data structure definition here ?

Thanks.
> 
> ...
> 
> > > > > +	__u8	uuid[16];
> > > > 
> > > > You just changed the type here, so what is that going to break in
> > > > userspace that depended on this being of a structure type and now it's
> > > > an array?
> > > 
> > > It's the same. The previous was hidden behind additional type level.
> > 
> > Same size, yes.  Same C structure definition, no.
> 
> It doesn't matter, see below.
> 
> > > > And no other kernel changes needed?  Shouldn't you warn if this field is
> > > > set?
> > > 
> > > No, as pointed out in the commit message kernel never ever used this.
> > 
> > That does not mean that userspace tools never did, right?  You are
> > changing the structure definition, what tool just broke?
> 
> The only tool has been amended like a year ago, so the answer is none.
> The commit message has links to the commits in question that made that
> amendment.
> 
> Maybe I should remove Fixes tags? In such case we will very much know
> that no old tools will be run on the new kernel.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko Nov. 17, 2022, 6:47 a.m. UTC | #8 
On Thu, Nov 17, 2022 at 09:03:48AM +0800, Fei Li wrote:
> On 2022-11-16 at 18:04:37 +0200, Andy Shevchenko wrote:

...

> ACRN does not use uuid recently. But before that, the old ACRN still uses it.
> So could we just change the uuid data structure definition here ?

That's what is meant by removing the Fixes tags.
Andy Shevchenko Nov. 17, 2022, 6:50 a.m. UTC | #9 
On Thu, Nov 17, 2022 at 08:47:10AM +0200, Andy Shevchenko wrote:
> On Thu, Nov 17, 2022 at 09:03:48AM +0800, Fei Li wrote:
> > On 2022-11-16 at 18:04:37 +0200, Andy Shevchenko wrote:

...

> > ACRN does not use uuid recently. But before that, the old ACRN still uses it.
> > So could we just change the uuid data structure definition here ?
> 
> That's what is meant by removing the Fixes tags.

Note, this change also removes the link to non-existing file.
diff mbox series

Patch

diff --git a/include/uapi/linux/acrn.h b/include/uapi/linux/acrn.h
index ccf47ed92500..04fa83647ae5 100644
--- a/include/uapi/linux/acrn.h
+++ b/include/uapi/linux/acrn.h
@@ -12,7 +12,6 @@ 
 #define _UAPI_ACRN_H
 
 #include <linux/types.h>
-#include <linux/uuid.h>
 
 #define ACRN_IO_REQUEST_MAX		16
 
@@ -186,7 +185,7 @@  struct acrn_ioreq_notify {
  * @reserved0:		Reserved and must be 0
  * @vcpu_num:		Number of vCPU in the VM. Return from hypervisor.
  * @reserved1:		Reserved and must be 0
- * @uuid:		UUID of the VM. Pass to hypervisor directly.
+ * @uuid:		Reserved (used to be UUID of the VM)
  * @vm_flag:		Flag of the VM creating. Pass to hypervisor directly.
  * @ioreq_buf:		Service VM GPA of I/O request buffer. Pass to
  *			hypervisor directly.
@@ -198,7 +197,7 @@  struct acrn_vm_creation {
 	__u16	reserved0;
 	__u16	vcpu_num;
 	__u16	reserved1;
-	guid_t	uuid;
+	__u8	uuid[16];
 	__u64	vm_flag;
 	__u64	ioreq_buf;
 	__u64	cpu_affinity;
diff --git a/samples/acrn/vm-sample.c b/samples/acrn/vm-sample.c
index b2dad47a77a0..7abd68b20153 100644
--- a/samples/acrn/vm-sample.c
+++ b/samples/acrn/vm-sample.c
@@ -29,8 +29,6 @@  static struct acrn_io_request *io_req_buf = (struct acrn_io_request *)io_request
 
 __u16 vcpu_num;
 __u16 vmid;
-/* POST_STANDARD_VM_UUID1, refer to https://github.com/projectacrn/acrn-hypervisor/blob/master/hypervisor/include/common/vm_uuids.h */
-guid_t vm_uuid = GUID_INIT(0x385479d2, 0xd625, 0xe811, 0x86, 0x4e, 0xcb, 0x7a, 0x18, 0xb3, 0x46, 0x43);
 
 int hsm_fd;
 int is_running = 1;
@@ -63,7 +61,6 @@  int main(int argc, char **argv)
 	}
 	hsm_fd = open("/dev/acrn_hsm", O_RDWR|O_CLOEXEC);
 
-	memcpy(&create_vm.uuid, &vm_uuid, 16);
 	create_vm.ioreq_buf = (__u64)io_req_buf;
 	ret = ioctl(hsm_fd, ACRN_IOCTL_CREATE_VM, &create_vm);
 	printf("Created VM! [%d]\n", ret);