[Differential] D14473: userboot: add callbacks to set unrestricted guest mode

[fabian.freyer_physik.tu-berlin.de (Fabian Freyer)]

fabian.freyer_physik.tu-berlin.de added 1 blocking reviewer(s): grehan.

REVISION DETAIL
  https://reviews.freebsd.org/D14473

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: fabian.freyer_physik.tu-berlin.de, imp, rgrimes, #bhyve, grehan
Cc: grehan, imp, freebsd-virtualization-list, #contributor_reviews_base
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

[Differential] D14473: userboot: add callbacks to set unrestricted guest mode

[grehan (Peter Grehan)]

grehan added a comment.


  So a comment on this: in general, api's are not added to FreeBSD that don't 
have base-system clients that use them, or for a good reason. I think this 
falls into the latter but I'd like to cut it down a bit.
  
  - can the get_unrestricted_guest() routine be removed ? There is an error 
return on the set, which seems like it can be used to determine if unrestricted 
mode is not available (e.g. that's how bhyve uses the ioctl).
  - is there a need for vcpu_reset() ? The BSP should be initialized to 
power-on state.. That could be a bug in bhyve and better to have it fixed there.

REVISION DETAIL
  https://reviews.freebsd.org/D14473

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: fabian.freyer_physik.tu-berlin.de, imp, rgrimes, #bhyve, grehan
Cc: grehan, imp, freebsd-virtualization-list, #contributor_reviews_base
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

[Differential] D14473: userboot: add callbacks to set unrestricted guest mode

[fabian.freyer_physik.tu-berlin.de (Fabian Freyer)]

fabian.freyer_physik.tu-berlin.de added a comment.


  In D14473#306827 , @grehan wrote:
  
  > So a comment on this: in general, api's are not added to FreeBSD that don't 
have base-system clients that use them, or for a good reason. I think this 
falls into the latter but I'd like to cut it down a bit.
  >
  > - can the get_unrestricted_guest() routine be removed ? There is an error 
return on the set, which seems like it can be used to determine if unrestricted 
mode is not available (e.g. that's how bhyve uses the ioctl).
  
  
  Yes, it can.
  
  > - is there a need for vcpu_reset() ? The BSP should be initialized to 
power-on state.. That could be a bug in bhyve and better to have it fixed there.
  
  Not necessarily, as everything in vcpu_reset() could also be accomplished 
with the other callbacks. I don't think bhyverun should call vcpu_reset(), as 
bhyveload sets up registers before. I guess this should happen before 
`loader_main` is called?

REVISION DETAIL
  https://reviews.freebsd.org/D14473

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: fabian.freyer_physik.tu-berlin.de, imp, rgrimes, #bhyve, grehan
Cc: grehan, imp, freebsd-virtualization-list, #contributor_reviews_base
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Call for testing bhyve cpu topology additions

[Rodney W. Grimes]

There is a new version of the CPU topology review up at
http://reviews.freebsd.org/D9930

I would like to ask that if people can test this and provide
feedback that they do so.

It has some undesired side effects on vm-bhyve and probably
other down stream tools, I am in the process of contacting
the vm-bhyve author to see what can be done to clean up
this output (if you are here please respond to this thread):

src-topo # vm list
NAMEDATASTORE   LOADER  CPUMEMORYVNC
  AUTOSTARTSTATE
issd30g1default bhyveload   cpus=8,sockets=2,cores=4,threads=1 
1024M -No   Stopped

Notice that due to the new CPU string being much more complicated than
the normal int it makes the output format ugly.  I have another change
to vm-bhyve that I would like to see too, and that is to move the NAME
to the end of the line and remove the 16 character limit.   I have done
that in my local copy already.  This string and its parsing are designed
to be Qemu compatible.

Here is a sample of my local vm-bhyve vm list output (no topology used):
/tmp # vm list
DATASTORE   LOADER  CPUMEMORYVNC  AUTOSTART
STATENAME
default bhyveload   1  128M  -No   
Stopped  fb-bld-10-amd64
default bhyveload   1  128M  -No   
Stopped  fb-bld-11-amd64
default bhyveload   1  128M  -No   
Stopped  fb-bld-11.0-p1-amd64
default bhyveload   1  128M  -No   
Stopped  fb-bld-11.0-p1-i386
default bhyveload   4  512M  -No   
Stopped  fb-bld-11_1-amd64
default bhyveload   4  512M  -No   
Stopped  fb-bld-11_1-i386
default bhyveload   2  256M  -No   
Running (30227)  fb-bld-head-amd64

Thoughts are to teach vm-bhyve to parse the string just as bhyve does
and only output the vCPU count.  Other thoughts I have are to have
it parse the string and output either vCPU if cores/threads is 1,
or a simple S/C/T string.

If you are a downstream maintainer of one of the other vm management packages
I am open to input.  The implemntation I have done should allow any existing
tool that treated -c as a string to use the new topology without changes.
This includes the in base vmrun.sh.

Also people using the sysctls:
hw.vmm.topology.cores_per_package: 1
hw.vmm.topology.threads_per_core: 1
can continue to do so at this time, but there is work in process to
deprecate these, that work includes making stable/11 emit a warning
message if they are used, and remove them in head/12.

If I can get some significant test results back I plan to commit
D9930 to ^head and merge it back to stable/11 3 days later.

Thanks,
-- 
Rod Grimes rgri...@freebsd.org
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Re: Call for testing bhyve cpu topology additions

[Peter Grehan]

I would like to ask that if people can test this and provide
feedback that they do so.


 And as mentioned in the review, I'd also like to see your Windows 
desktop guest test results with this change.



If I can get some significant test results back I plan to commit
D9930 to ^head and merge it back to stable/11 3 days later.


 Standard MFC time is 3 weeks.

later,

Peter.
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Re: Call for testing bhyve cpu topology additions

[Rodney W. Grimes]

> > I would like to ask that if people can test this and provide
> > feedback that they do so.
> 
>   And as mentioned in the review, I'd also like to see your Windows 
> desktop guest test results with this change.

I do not run any windows in bhyve as bhyve can not run the
windows I use due to missing/broken ATA support.
The person I was helping with bhyve windows regression tests has
become unavaliable.

> > If I can get some significant test results back I plan to commit
> > D9930 to ^head and merge it back to stable/11 3 days later.
> 
>   Standard MFC time is 3 weeks.

Can you point to this some place?  My understanding is that
MFC is at the discretion of the committer, and the only
thing the big list of rules says:

6. Changes go to FreeBSD-CURRENT before FreeBSD-STABLE unless
specifically permitted by the release engineer or unless they
are not applicable to FreeBSD-CURRENT. Any non-trivial or
non-urgent change which is applicable should also be allowed
to sit in FreeBSD-CURRENT for at least 3 days before merging
so that it can be given sufficient testing.

I am making a wide call for testing, above and beyond the
normal process already.  I am also about to pull this back
to my own 11.1 systems to ensure that I spot any merge
problems and if need be attach an 11.1 and 11/stable patch
to the diffential.

There is also a call for testing going out at byhvecon,
and there has already been a year to test on this,
though perhaps not in final form, at least in functional
form.

Regards,
-- 
Rod Grimes rgri...@freebsd.org
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Re: Call for testing bhyve cpu topology additions

[Peter Grehan]

   And as mentioned in the review, I'd also like to see your Windows
desktop guest test results with this change.


I do not run any windows in bhyve as bhyve can not run the
windows I use due to missing/broken ATA support.
The person I was helping with bhyve windows regression tests has
become unavaliable.


 I'm staggered by this. The *only reason* for having this change is to 
get past the 1/2 socket limitation when running Windows desktop guests. 
It has no impact on any other guests, whatsoever.


 You can easily download an eval of Windows 10 to try this out with. 
You do not (and have never) required ATA support to run Windows on bhyve.



   Standard MFC time is 3 weeks.


Can you point to this some place?  My understanding is that
MFC is at the discretion of the committer, and the only
thing the big list of rules says:


 This is project culture, which you should have seen observing MFC 
times for commits.


 And why the rush ? I'm yet to understand what the urgency for this 
work is ? Who is demanding it ?


later,

Peter.
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Re: Call for testing bhyve cpu topology additions

[Rodney W. Grimes]

> >>And as mentioned in the review, I'd also like to see your Windows
> >> desktop guest test results with this change.
> > 
> > I do not run any windows in bhyve as bhyve can not run the
> > windows I use due to missing/broken ATA support.
> > The person I was helping with bhyve windows regression tests has
> > become unavaliable.
> 
>   I'm staggered by this. The *only reason* for having this change is to 
> get past the 1/2 socket limitation when running Windows desktop guests. 
> It has no impact on any other guests, whatsoever.

I shall iterate again, this change makes no change to what the guests
sees if you use the old method sysctl hw.vmm.topology.cores_per_package
or hw.vmm.topology.threads_per_core to set these values, it is
purely a interface enhancement that makes these tuneables easy
to access from userland bhyve(8).

A guest can not tell the diffence in what way these are set.
If hw.vmm.topology.* needs testing thats not on me, that is
an existing problem, and one that has existed for far too long.

> 
>   You can easily download an eval of Windows 10 to try this out with. 
> You do not (and have never) required ATA support to run Windows on bhyve.

I have made a call for testing, whats your issue?
Are you trying to force me to do that testing?

> 
> >>Standard MFC time is 3 weeks.
> > 
> > Can you point to this some place?  My understanding is that
> > MFC is at the discretion of the committer, and the only
> > thing the big list of rules says:
> 
>   This is project culture, which you should have seen observing MFC 
> times for commits.

And I consider this change rather trivial and with near 0 risk,
so choose to use a 3 day MFC timer for it.  The worse that can
happen is the user would have to use the old sysctls which I
left in place with full compatibility.

>   And why the rush ? I'm yet to understand what the urgency for this 
> work is ? Who is demanding it ?

The users have been wanting this for well over a year, it was
a frequently requested item when I wrote it.  It is long overdue.

You seem to be raising a bar higher than any other part of
FreeBSD for this rather simple user command enhancement,
can I ask what your objection is?  If you think the topology
seen by the guest is going to be bad, that bug exists today
for anyone trying to use the sysctl's, perhaps we can find
that there are bugs if we get this in the hands of the user
rather than yet again delay a new feature for what appears
to be tests of code paths very minimally effected (change
of a variable name) by this patch.

Regards,
-- 
Rod Grimes rgri...@freebsd.org
___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"

Re: Call for testing bhyve cpu topology additions

[Peter Grehan]

I shall iterate again, this change makes no change to what the guests
sees if you use the old method sysctl hw.vmm.topology.cores_per_package
or hw.vmm.topology.threads_per_core to set these values, it is
purely a interface enhancement that makes these tuneables easy
to access from userland bhyve(8).


 Those sysctls were an undocumented workaround with no error checking.

 You are making this a documented part of bhyve,


A guest can not tell the diffence in what way these are set.
If hw.vmm.topology.* needs testing thats not on me, that is
an existing problem, and one that has existed for far too long.


 Ah, no, the testing is on you, not the user community.


   You can easily download an eval of Windows 10 to try this out with.
You do not (and have never) required ATA support to run Windows on bhyve.


I have made a call for testing, whats your issue?
Are you trying to force me to do that testing?


 At a minimum, you are expected to test changes that you expect to commit.


And I consider this change rather trivial and with near 0 risk,


 You've never made a commit to bhyve but somehow feel qualified to make 
risk assessments.



   And why the rush ? I'm yet to understand what the urgency for this
work is ? Who is demanding it ?


The users have been wanting this for well over a year, it was
a frequently requested item when I wrote it.  It is long overdue.


 Right, so 3 weeks for MFC is perfectly acceptable in that case.


You seem to be raising a bar higher than any other part of
FreeBSD for this rather simple user command enhancement,
can I ask what your objection is?


 The fact that you seem to think testing this is someone else's 
problem, in a subsystem where rigorous testing is a requirement.


later,

Peter.

___
freebsd-virtualization@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-virtualization
To unsubscribe, send any mail to 
"freebsd-virtualization-unsubscr...@freebsd.org"