Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread John Baldwin 
On 3/15/19 2:36 PM, Chuck Tuffli wrote:
> I was contacted by a bhyve user who mentioned that Windows didn't seem
> to like bhyve's NVMe emulation. This change doesn't fix that, but this
> is one of a handful of changes inspired by qemu. While trying to
> reverse engineer why Windows is grumpy, I've run across several
> comments in the qemu code which make claims about what Windows needs
> to see before starting a device. This was one of those, and (at the
> time) seemed innocuous enough to commit.

Ok.  It seems like a bit of an odd requirement, but perhaps Windows PCI-e error
handling stuff assumes it is present?  It might be worth expanding the comment
to note that as the reason?

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Chuck Tuffli 
Apologies all the way around. I was ignorant about the maintainer for
this code and goofed. See inline for other comments.

On Fri, Mar 15, 2019 at 9:28 AM John Baldwin  wrote:
>
> On 3/14/19 10:24 PM, Conrad Meyer wrote:
> > On Thu, Mar 14, 2019 at 8:06 PM Andrew Thompson  wrote:
> >>
> >> On Fri, 15 Mar 2019 at 15:11, Chuck Tuffli  wrote:
> >>> bzero(, sizeof(pciecap));
> > ...
> >>> +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> >>
> >> If the message you say 'set the bit' but you are overwriting the whole 
> >> variable, is this intended?
> >
> > Looks like it was zero before.  So yeah, it sets the bit.
>
> It would probably be cleaner for future changes to make it a |=, but that's a
> tiny nit.  style(9) wants a blank line before the comment as well.

Happy to make those changes

> I hadn't approved it yet only because I hadn't gone and dug through my PCIe
> books / specs to see what this bit is and confirm it is required.
>
> OTOH, it's not clear to me that bhyve PCI-e devices don't want to just be 1.0a
> devices as a lowest common denominator to be as accommodating to as wide 
> variety
> of OS's as possible.
>
> One thing I didn't see in a review was a reason for why to make this change?
> Does some OS reject devices without this bit set or is it just based on 
> reading
> the spec?  bhyve doesn't assert any PCI-e errors for virtual devices, so
> this bit is pretty meaningless.

I was contacted by a bhyve user who mentioned that Windows didn't seem
to like bhyve's NVMe emulation. This change doesn't fix that, but this
is one of a handful of changes inspired by qemu. While trying to
reverse engineer why Windows is grumpy, I've run across several
comments in the qemu code which make claims about what Windows needs
to see before starting a device. This was one of those, and (at the
time) seemed innocuous enough to commit.

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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Warner Losh 
On Fri, Mar 15, 2019 at 11:27 AM Rodney W. Grimes 
wrote:

> > On Fri, Mar 15, 2019 at 9:56 AM Rodney W. Grimes <
> free...@gndrsh.dnsmgr.net>
> > wrote:
> >
> > > > On Thu, Mar 14, 2019 at 8:32 PM Rodney W. Grimes <
> > > free...@gndrsh.dnsmgr.net>
> > > > wrote:
> > > >
> > > > > > Author: chuck
> > > > > > Date: Fri Mar 15 02:11:28 2019
> > > > > > New Revision: 345171
> > > > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > > > >
> > > > > > Log:
> > > > > >   Fix bhyve PCIe capability emulation
> > > > > >
> > > > > >   PCIe devices starting with version 1.1 must set the Role-Based
> > > Error
> > > > > >   Reporting bit.
> > > > > >
> > > > > >   And while we're in the neighborhood, generalize the code
> assigning
> > > the
> > > > > >   device type.
> > > > > >
> > > > > >   Reviewed by:imp, araujo, rgrimes
> > > > > >   Approved by:imp (mentor)
> > > > > >   MFC after:  1 week
> > > > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > > > >
> > > > > This code requires maintainer approval before a commit,
> > > > > though this was well reviewed that doesnt exclude it
> > > > > from the MAINTAINERS entry.
> > > > >
> > > > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > > > this is just a heads up FYI for future commits.
> > > > >
> > > > > Bhyve code has been and still is under a fairly tight
> > > > > MAINTAINER status.
> > > > >
> > > >
> > > > There is no such thing as a hard lock in FreeBSD. This sounds like
> you
> > > are
> > > > advocating for that, but that's not the case.
> > > >
> > > > Stop this stupid nitpicking for single line commits. We don't have
> that
> ^^
>
> Thank you for calling my actions stupid, in effect demoralizing me with
> the label that includes.  I may nit pick, but I never call people degrading
> names on a public list.


I said your actions (specifically the nitpicking) were stupid. I never
called you stupid, and don't think you are stupid.

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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Marcelo Araujo 
Em sáb, 16 de mar de 2019 às 01:27, Rodney W. Grimes <
free...@gndrsh.dnsmgr.net> escreveu:

> > On Fri, Mar 15, 2019 at 9:56 AM Rodney W. Grimes <
> free...@gndrsh.dnsmgr.net>
> > wrote:
> >
> > > > On Thu, Mar 14, 2019 at 8:32 PM Rodney W. Grimes <
> > > free...@gndrsh.dnsmgr.net>
> > > > wrote:
> > > >
> > > > > > Author: chuck
> > > > > > Date: Fri Mar 15 02:11:28 2019
> > > > > > New Revision: 345171
> > > > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > > > >
> > > > > > Log:
> > > > > >   Fix bhyve PCIe capability emulation
> > > > > >
> > > > > >   PCIe devices starting with version 1.1 must set the Role-Based
> > > Error
> > > > > >   Reporting bit.
> > > > > >
> > > > > >   And while we're in the neighborhood, generalize the code
> assigning
> > > the
> > > > > >   device type.
> > > > > >
> > > > > >   Reviewed by:imp, araujo, rgrimes
> > > > > >   Approved by:imp (mentor)
> > > > > >   MFC after:  1 week
> > > > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > > > >
> > > > > This code requires maintainer approval before a commit,
> > > > > though this was well reviewed that doesnt exclude it
> > > > > from the MAINTAINERS entry.
> > > > >
> > > > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > > > this is just a heads up FYI for future commits.
> > > > >
> > > > > Bhyve code has been and still is under a fairly tight
> > > > > MAINTAINER status.
> > > > >
> > > >
> > > > There is no such thing as a hard lock in FreeBSD. This sounds like
> you
> > > are
> > > > advocating for that, but that's not the case.
> > > >
> > > > Stop this stupid nitpicking for single line commits. We don't have
> that
> ^^
>
> Thank you for calling my actions stupid, in effect demoralizing me with
> the label that includes.  I may nit pick, but I never call people degrading
> names on a public list.
>
> Also it only takes a single like to make a bug or problem,
> it would help to not consider single line changes any less or
> any more important or potentially damaging.
>
> > > > culture any more and it's really pissing a lot of people off.
> > > >
> > > > The MAINTAINERS file even says this:
> > > >
> > > > Please note that the content of this file is strictly advisory.
> > > >
> > > > And the entry for bhyve doesn't say things are mandatory, just
> requested.
> > > >
> > > > Jumping on people's case like this, for a review you yourself were
> on and
> > > > approved but made no mention of seeking further review / approval, is
> > > > demotivating and toxic. Please stop.
> > >
> > > I explicitly DID add jhb to the review.
> > > I also explicitly did not mark the bhyve# box that is added by
> > > the hearald rules.
> > >
> > > I did not jump on him, I informed him of the entry, and told him to
> leave
> > > it.
> > > You how ever have infact jumped on me, repeatedly, if you want to talk
> > > about
> > > discouraging tones of behavior I suggest you look at yourself as well.
> > >
> >
> > Weird that so many other people in the thread read it the same way that I
> > did, and not only this time. That suggests that you're not aware that
> your
> > behavior is annoying others, which I contend is a problem that needs
> > looking into.
>
> If you have a pre conceived notion or opinion about anything I say,
> and I contend that many do, you well always hear it in that tone.  This is
> the rose colored glasses problem.  I can not fix that what you hear is
> not what I said.
>
> I speak frankly and without political or other polish to my words,
> which at times do make them sound harsh or overly direct.  I think
> we both actually do that, and, imho, that is better than trying to
> sugar coat stuff and be all polite and indirect about things.
>
> > As always, I'm open to constructive, actionable feedback about my
> actions.
>
> I have tried above.
>
> > Warner
> > > > Warner
> > > > >   head/usr.sbin/bhyve/pci_emul.c
> > > > > >
> > > > > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > > > >
> > > > >
> > >
> ==
> > > > > > --- head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:27 2019
> > > > > (r345170)
> > > > > > +++ head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:28 2019
> > > > > (r345171)
> > > > > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst
> *pi, int
> > > > > type)
> > > > > >   bzero(, sizeof(pciecap));
> > > > > >
> > > > > >   pciecap.capid = PCIY_EXPRESS;
> > > > > > - pciecap.pcie_capabilities = PCIECAP_VERSION |
> > > PCIEM_TYPE_ROOT_PORT;
> > > > > > + pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > > > > + /* Devices starting with version 1.1 must set the RBER bit
> */
> > > > > > + if (PCIECAP_VERSION >= 1)
> > > > > > + pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > > > >   pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > > > >

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Rodney W. Grimes 
> On Fri, Mar 15, 2019 at 9:56 AM Rodney W. Grimes 
> wrote:
> 
> > > On Thu, Mar 14, 2019 at 8:32 PM Rodney W. Grimes <
> > free...@gndrsh.dnsmgr.net>
> > > wrote:
> > >
> > > > > Author: chuck
> > > > > Date: Fri Mar 15 02:11:28 2019
> > > > > New Revision: 345171
> > > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > > >
> > > > > Log:
> > > > >   Fix bhyve PCIe capability emulation
> > > > >
> > > > >   PCIe devices starting with version 1.1 must set the Role-Based
> > Error
> > > > >   Reporting bit.
> > > > >
> > > > >   And while we're in the neighborhood, generalize the code assigning
> > the
> > > > >   device type.
> > > > >
> > > > >   Reviewed by:imp, araujo, rgrimes
> > > > >   Approved by:imp (mentor)
> > > > >   MFC after:  1 week
> > > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > > >
> > > > This code requires maintainer approval before a commit,
> > > > though this was well reviewed that doesnt exclude it
> > > > from the MAINTAINERS entry.
> > > >
> > > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > > this is just a heads up FYI for future commits.
> > > >
> > > > Bhyve code has been and still is under a fairly tight
> > > > MAINTAINER status.
> > > >
> > >
> > > There is no such thing as a hard lock in FreeBSD. This sounds like you
> > are
> > > advocating for that, but that's not the case.
> > >
> > > Stop this stupid nitpicking for single line commits. We don't have that
^^

Thank you for calling my actions stupid, in effect demoralizing me with
the label that includes.  I may nit pick, but I never call people degrading
names on a public list.

Also it only takes a single like to make a bug or problem,
it would help to not consider single line changes any less or
any more important or potentially damaging.

> > > culture any more and it's really pissing a lot of people off.
> > >
> > > The MAINTAINERS file even says this:
> > >
> > > Please note that the content of this file is strictly advisory.
> > >
> > > And the entry for bhyve doesn't say things are mandatory, just requested.
> > >
> > > Jumping on people's case like this, for a review you yourself were on and
> > > approved but made no mention of seeking further review / approval, is
> > > demotivating and toxic. Please stop.
> >
> > I explicitly DID add jhb to the review.
> > I also explicitly did not mark the bhyve# box that is added by
> > the hearald rules.
> >
> > I did not jump on him, I informed him of the entry, and told him to leave
> > it.
> > You how ever have infact jumped on me, repeatedly, if you want to talk
> > about
> > discouraging tones of behavior I suggest you look at yourself as well.
> >
> 
> Weird that so many other people in the thread read it the same way that I
> did, and not only this time. That suggests that you're not aware that your
> behavior is annoying others, which I contend is a problem that needs
> looking into.

If you have a pre conceived notion or opinion about anything I say,
and I contend that many do, you well always hear it in that tone.  This is
the rose colored glasses problem.  I can not fix that what you hear is
not what I said.

I speak frankly and without political or other polish to my words,
which at times do make them sound harsh or overly direct.  I think
we both actually do that, and, imho, that is better than trying to
sugar coat stuff and be all polite and indirect about things.

> As always, I'm open to constructive, actionable feedback about my actions.

I have tried above.  

> Warner
> > > Warner
> > > >   head/usr.sbin/bhyve/pci_emul.c
> > > > >
> > > > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > > >
> > > >
> > ==
> > > > > --- head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:27 2019
> > > > (r345170)
> > > > > +++ head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:28 2019
> > > > (r345171)
> > > > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int
> > > > type)
> > > > >   bzero(, sizeof(pciecap));
> > > > >
> > > > >   pciecap.capid = PCIY_EXPRESS;
> > > > > - pciecap.pcie_capabilities = PCIECAP_VERSION |
> > PCIEM_TYPE_ROOT_PORT;
> > > > > + pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > > > + /* Devices starting with version 1.1 must set the RBER bit */
> > > > > + if (PCIECAP_VERSION >= 1)
> > > > > + pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > > >   pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > > >   pciecap.link_status = 0x11; /* gen1, x1 */
> > > > >
> > > > >
> > > > >
> > > >
> > > > --
> > > > Rod Grimes
> > > > rgri...@freebsd.org
> > > >
> > > >
> >
> > --
> > Rod Grimes
> > rgri...@freebsd.org
> >

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Warner Losh 
On Fri, Mar 15, 2019 at 9:56 AM Rodney W. Grimes 
wrote:

> > On Thu, Mar 14, 2019 at 8:32 PM Rodney W. Grimes <
> free...@gndrsh.dnsmgr.net>
> > wrote:
> >
> > > > Author: chuck
> > > > Date: Fri Mar 15 02:11:28 2019
> > > > New Revision: 345171
> > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > >
> > > > Log:
> > > >   Fix bhyve PCIe capability emulation
> > > >
> > > >   PCIe devices starting with version 1.1 must set the Role-Based
> Error
> > > >   Reporting bit.
> > > >
> > > >   And while we're in the neighborhood, generalize the code assigning
> the
> > > >   device type.
> > > >
> > > >   Reviewed by:imp, araujo, rgrimes
> > > >   Approved by:imp (mentor)
> > > >   MFC after:  1 week
> > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > >
> > > This code requires maintainer approval before a commit,
> > > though this was well reviewed that doesnt exclude it
> > > from the MAINTAINERS entry.
> > >
> > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > this is just a heads up FYI for future commits.
> > >
> > > Bhyve code has been and still is under a fairly tight
> > > MAINTAINER status.
> > >
> >
> > There is no such thing as a hard lock in FreeBSD. This sounds like you
> are
> > advocating for that, but that's not the case.
> >
> > Stop this stupid nitpicking for single line commits. We don't have that
> > culture any more and it's really pissing a lot of people off.
> >
> > The MAINTAINERS file even says this:
> >
> > Please note that the content of this file is strictly advisory.
> >
> > And the entry for bhyve doesn't say things are mandatory, just requested.
> >
> > Jumping on people's case like this, for a review you yourself were on and
> > approved but made no mention of seeking further review / approval, is
> > demotivating and toxic. Please stop.
>
> I explicitly DID add jhb to the review.
> I also explicitly did not mark the bhyve# box that is added by
> the hearald rules.
>
> I did not jump on him, I informed him of the entry, and told him to leave
> it.
> You how ever have infact jumped on me, repeatedly, if you want to talk
> about
> discouraging tones of behavior I suggest you look at yourself as well.
>

Weird that so many other people in the thread read it the same way that I
did, and not only this time. That suggests that you're not aware that your
behavior is annoying others, which I contend is a problem that needs
looking into.

As always, I'm open to constructive, actionable feedback about my actions.

Warner


> > Warner
> >
> > >   head/usr.sbin/bhyve/pci_emul.c
> > > >
> > > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > >
> > >
> ==
> > > > --- head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:27 2019
> > > (r345170)
> > > > +++ head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:28 2019
> > > (r345171)
> > > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int
> > > type)
> > > >   bzero(, sizeof(pciecap));
> > > >
> > > >   pciecap.capid = PCIY_EXPRESS;
> > > > - pciecap.pcie_capabilities = PCIECAP_VERSION |
> PCIEM_TYPE_ROOT_PORT;
> > > > + pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > > + /* Devices starting with version 1.1 must set the RBER bit */
> > > > + if (PCIECAP_VERSION >= 1)
> > > > + pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > >   pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > >   pciecap.link_status = 0x11; /* gen1, x1 */
> > > >
> > > >
> > > >
> > >
> > > --
> > > Rod Grimes
> > > rgri...@freebsd.org
> > >
> > >
>
> --
> Rod Grimes
> rgri...@freebsd.org
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Rodney W. Grimes 
> Em s?b, 16 de mar de 2019 ?s 00:03, Rodney W. Grimes <
> free...@gndrsh.dnsmgr.net> escreveu:
> 
> > > Em sex, 15 de mar de 2019 ?s 22:12, Ian Lepore 
> > escreveu:
> > >
> > > > On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:
> > > > > > Author: chuck
> > > > > > Date: Fri Mar 15 02:11:28 2019
> > > > > > New Revision: 345171
> > > > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > > > >
> > > > > > Log:
> > > > > >   Fix bhyve PCIe capability emulation
> > > > > >
> > > > > >   PCIe devices starting with version 1.1 must set the Role-Based
> > > > > > Error
> > > > > >   Reporting bit.
> > > > > >
> > > > > >   And while we're in the neighborhood, generalize the code
> > > > > > assigning the
> > > > > >   device type.
> > > > > >
> > > > > >   Reviewed by:  imp, araujo, rgrimes
> > > > > >   Approved by:  imp (mentor)
> > > > > >   MFC after:1 week
> > > > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > > > >
> > > > > This code requires maintainer approval before a commit,
> > > > > though this was well reviewed that doesnt exclude it
> > > > > from the MAINTAINERS entry.
> > > > >
> > > >
> > > > Where exactly does it say that in MAINTAINERS?  As another victim of
> > > > this sort of drive-by lynching after making a trivial bhyve change I
> > > > pretty seriously object to a vague and meaningless entry in MAINTAINERS
> > > > being used to pounce on anyone who dares to touch the precious bhyve
> > > > code.
> > > >
> > >
> > > There is a new entry on MAINTAINERS:
> > > https://svnweb.freebsd.org/base?view=revision=344631
> > >
> > >
> > > >
> > > > There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
> > > > There is an entry for vmm(4), which to me does not say anything about
> > > > bhyve, yet somehow everybody is supposed to know what it means and
> > > > what-all territory it covers?
> > > >
> > > > IMO, this sort of hyper-proprietary pouncing on everyone who dares
> > > > change a single line of code is not productive.  It is HIGHLY de-
> > > > motivating.  Large sweeping design changes are one thing, but pouncing
> > > > on every tiny minor commit is just not helpful.
> > > >
> > >
> > > +1
> > >
> > > I got so frustrated with it recently that I have decided to don't
> > > contribute with bhyve anymore, perhaps even with FreeBSD.
> > > I still have some people under mentorship that I intend to finish and
> > then
> > > probably I will phase out.
> >
> > Your failure to get reviews, and infact even abandon one that had
> > negative advice as to the validity of your suggested change and
> > committing it anyway is more likely the cause here.
> >
> > You also committed code with no review at all that had to be reverted
> > after the bugs it caused were found by an external down stream consumers
> > of the bhyve code.
> >
> > You had code reverted by core due to a external attribution request,
> > which had you been attending the bi monthly bhyve calls you would of
> > known was an issue.
> >
> > I would suggest these are the reasons your feeling angry, and that
> > I infact tried to reach out to jhb to discuss some of these earlier
> > but that reach out was never returned.  I under stand your frustration,
> > you are just wanting to do with best thing you can for the project
> > and bhyve, can we try to find a better resolution to this situation
> > than your exit?
> >
> 
> 
> 
> 
> >
> > > > -- Ian
> > > >
> > > > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > > > this is just a heads up FYI for future commits.
> > > > >
> > > > > Bhyve code has been and still is under a fairly tight
> > > > > MAINTAINER status.
> > > > >
> > > > > > Modified:
> > > > > >   head/usr.sbin/bhyve/pci_emul.c
> > > > > >
> > > > > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > > > > ===
> > > > > > ===
> > > > > > --- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019
> >   (r3
> > > > > > 45170)
> > > > > > +++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019
> >   (r3
> > > > > > 45171)
> > > > > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
> > > > > > int type)
> > > > > > bzero(, sizeof(pciecap));
> > > > > >
> > > > > > pciecap.capid = PCIY_EXPRESS;
> > > > > > -   pciecap.pcie_capabilities = PCIECAP_VERSION |
> > > > > > PCIEM_TYPE_ROOT_PORT;
> > > > > > +   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > > > > +   /* Devices starting with version 1.1 must set the RBER bit */
> > > > > > +   if (PCIECAP_VERSION >= 1)
> > > > > > +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > > > > pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > > > > pciecap.link_status = 0x11; /* gen1, x1 */
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > >
> > > --
> > >
> > > --
> > > Marcelo Araujo

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread John Baldwin 
On 3/15/19 9:27 AM, John Baldwin wrote:
> On 3/14/19 10:24 PM, Conrad Meyer wrote:
>> On Thu, Mar 14, 2019 at 8:06 PM Andrew Thompson  wrote:
>>>
>>> On Fri, 15 Mar 2019 at 15:11, Chuck Tuffli  wrote:
 bzero(, sizeof(pciecap));
>> ...
 +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
>>>
>>> If the message you say 'set the bit' but you are overwriting the whole 
>>> variable, is this intended?
>>
>> Looks like it was zero before.  So yeah, it sets the bit.
> 
> It would probably be cleaner for future changes to make it a |=, but that's a
> tiny nit.  style(9) wants a blank line before the comment as well.
> 
> I hadn't approved it yet only because I hadn't gone and dug through my PCIe
> books / specs to see what this bit is and confirm it is required.
> 
> OTOH, it's not clear to me that bhyve PCI-e devices don't want to just be 1.0a
> devices as a lowest common denominator to be as accommodating to as wide 
> variety
> of OS's as possible.
> 
> One thing I didn't see in a review was a reason for why to make this change?
> Does some OS reject devices without this bit set or is it just based on 
> reading
> the spec?  bhyve doesn't assert any PCI-e errors for virtual devices, so
> this bit is pretty meaningless.

On the topic of a hard lock, the intention of "Review requested" is not to
enforce a hard lock, but to request a heads up so that work can be coordinated.
I think committing this was ok given other people ok'd the change, though I
think I still I want some answers as to the "why" this is needed to think about
if we actually want the change or not.

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread John Baldwin 
On 3/14/19 10:24 PM, Conrad Meyer wrote:
> On Thu, Mar 14, 2019 at 8:06 PM Andrew Thompson  wrote:
>>
>> On Fri, 15 Mar 2019 at 15:11, Chuck Tuffli  wrote:
>>> bzero(, sizeof(pciecap));
> ...
>>> +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
>>
>> If the message you say 'set the bit' but you are overwriting the whole 
>> variable, is this intended?
> 
> Looks like it was zero before.  So yeah, it sets the bit.

It would probably be cleaner for future changes to make it a |=, but that's a
tiny nit.  style(9) wants a blank line before the comment as well.

I hadn't approved it yet only because I hadn't gone and dug through my PCIe
books / specs to see what this bit is and confirm it is required.

OTOH, it's not clear to me that bhyve PCI-e devices don't want to just be 1.0a
devices as a lowest common denominator to be as accommodating to as wide variety
of OS's as possible.

One thing I didn't see in a review was a reason for why to make this change?
Does some OS reject devices without this bit set or is it just based on reading
the spec?  bhyve doesn't assert any PCI-e errors for virtual devices, so
this bit is pretty meaningless.

-- 
John Baldwin
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Marcelo Araujo 
Em sáb, 16 de mar de 2019 às 00:03, Rodney W. Grimes <
free...@gndrsh.dnsmgr.net> escreveu:

> > Em sex, 15 de mar de 2019 ?s 22:12, Ian Lepore 
> escreveu:
> >
> > > On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:
> > > > > Author: chuck
> > > > > Date: Fri Mar 15 02:11:28 2019
> > > > > New Revision: 345171
> > > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > > >
> > > > > Log:
> > > > >   Fix bhyve PCIe capability emulation
> > > > >
> > > > >   PCIe devices starting with version 1.1 must set the Role-Based
> > > > > Error
> > > > >   Reporting bit.
> > > > >
> > > > >   And while we're in the neighborhood, generalize the code
> > > > > assigning the
> > > > >   device type.
> > > > >
> > > > >   Reviewed by:  imp, araujo, rgrimes
> > > > >   Approved by:  imp (mentor)
> > > > >   MFC after:1 week
> > > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > > >
> > > > This code requires maintainer approval before a commit,
> > > > though this was well reviewed that doesnt exclude it
> > > > from the MAINTAINERS entry.
> > > >
> > >
> > > Where exactly does it say that in MAINTAINERS?  As another victim of
> > > this sort of drive-by lynching after making a trivial bhyve change I
> > > pretty seriously object to a vague and meaningless entry in MAINTAINERS
> > > being used to pounce on anyone who dares to touch the precious bhyve
> > > code.
> > >
> >
> > There is a new entry on MAINTAINERS:
> > https://svnweb.freebsd.org/base?view=revision=344631
> >
> >
> > >
> > > There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
> > > There is an entry for vmm(4), which to me does not say anything about
> > > bhyve, yet somehow everybody is supposed to know what it means and
> > > what-all territory it covers?
> > >
> > > IMO, this sort of hyper-proprietary pouncing on everyone who dares
> > > change a single line of code is not productive.  It is HIGHLY de-
> > > motivating.  Large sweeping design changes are one thing, but pouncing
> > > on every tiny minor commit is just not helpful.
> > >
> >
> > +1
> >
> > I got so frustrated with it recently that I have decided to don't
> > contribute with bhyve anymore, perhaps even with FreeBSD.
> > I still have some people under mentorship that I intend to finish and
> then
> > probably I will phase out.
>
> Your failure to get reviews, and infact even abandon one that had
> negative advice as to the validity of your suggested change and
> committing it anyway is more likely the cause here.
>
> You also committed code with no review at all that had to be reverted
> after the bugs it caused were found by an external down stream consumers
> of the bhyve code.
>
> You had code reverted by core due to a external attribution request,
> which had you been attending the bi monthly bhyve calls you would of
> known was an issue.
>
> I would suggest these are the reasons your feeling angry, and that
> I infact tried to reach out to jhb to discuss some of these earlier
> but that reach out was never returned.  I under stand your frustration,
> you are just wanting to do with best thing you can for the project
> and bhyve, can we try to find a better resolution to this situation
> than your exit?
>




>
> > > -- Ian
> > >
> > > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > > this is just a heads up FYI for future commits.
> > > >
> > > > Bhyve code has been and still is under a fairly tight
> > > > MAINTAINER status.
> > > >
> > > > > Modified:
> > > > >   head/usr.sbin/bhyve/pci_emul.c
> > > > >
> > > > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > > > ===
> > > > > ===
> > > > > --- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019
>   (r3
> > > > > 45170)
> > > > > +++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019
>   (r3
> > > > > 45171)
> > > > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
> > > > > int type)
> > > > > bzero(, sizeof(pciecap));
> > > > >
> > > > > pciecap.capid = PCIY_EXPRESS;
> > > > > -   pciecap.pcie_capabilities = PCIECAP_VERSION |
> > > > > PCIEM_TYPE_ROOT_PORT;
> > > > > +   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > > > +   /* Devices starting with version 1.1 must set the RBER bit */
> > > > > +   if (PCIECAP_VERSION >= 1)
> > > > > +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > > > pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > > > pciecap.link_status = 0x11; /* gen1, x1 */
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> > --
> >
> > --
> > Marcelo Araujo(__)ara...@freebsd.org
> > \\\'',)http://www.FreeBSD.org    \/  \ ^
> > Power To Server. .\. /_)
>
> --
> Rod Grimes
> rgri...@freebsd.org
>

Sorry Rod, I don't want to be rude, but even though I'm not a native
English speaker, it hurts my eyes

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Pedro Giffuni 


On 15/03/2019 11:03, Rodney W. Grimes wrote:

Em sex, 15 de mar de 2019 ?s 22:12, Ian Lepore  escreveu:


On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:

Author: chuck
Date: Fri Mar 15 02:11:28 2019
New Revision: 345171
URL: https://svnweb.freebsd.org/changeset/base/345171

Log:
   Fix bhyve PCIe capability emulation

   PCIe devices starting with version 1.1 must set the Role-Based
Error
   Reporting bit.

   And while we're in the neighborhood, generalize the code
assigning the
   device type.

   Reviewed by:  imp, araujo, rgrimes
   Approved by:  imp (mentor)
   MFC after:1 week
   Differential Revision: https://reviews.freebsd.org/D19580

This code requires maintainer approval before a commit,
though this was well reviewed that doesnt exclude it
from the MAINTAINERS entry.


Where exactly does it say that in MAINTAINERS?  As another victim of
this sort of drive-by lynching after making a trivial bhyve change I
pretty seriously object to a vague and meaningless entry in MAINTAINERS
being used to pounce on anyone who dares to touch the precious bhyve
code.


There is a new entry on MAINTAINERS:
https://svnweb.freebsd.org/base?view=revision=344631



There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
There is an entry for vmm(4), which to me does not say anything about
bhyve, yet somehow everybody is supposed to know what it means and
what-all territory it covers?

IMO, this sort of hyper-proprietary pouncing on everyone who dares
change a single line of code is not productive.  It is HIGHLY de-
motivating.  Large sweeping design changes are one thing, but pouncing
on every tiny minor commit is just not helpful.


+1

I got so frustrated with it recently that I have decided to don't
contribute with bhyve anymore, perhaps even with FreeBSD.
I still have some people under mentorship that I intend to finish and then
probably I will phase out.

Your failure to get reviews, and infact even abandon one that had
negative advice as to the validity of your suggested change and
committing it anyway is more likely the cause here.


I will have to add  to the choir here: getting reviews is not mandatory 
and I hope they will never be.


Reviewed code is likely to have less errors but sometimes we just need 
to get things done.


Look, for example, the case of the automounting daemon amd(8), which we 
have been planning to remove for some years: your unfortunate 
intervention stopped it from getting removed from the system for how 
many months(?) stopping progress there altogether.


Pedro.





You also committed code with no review at all that had to be reverted
after the bugs it caused were found by an external down stream consumers
of the bhyve code.

You had code reverted by core due to a external attribution request,
which had you been attending the bi monthly bhyve calls you would of
known was an issue.

I would suggest these are the reasons your feeling angry, and that
I infact tried to reach out to jhb to discuss some of these earlier
but that reach out was never returned.  I under stand your frustration,
you are just wanting to do with best thing you can for the project
and bhyve, can we try to find a better resolution to this situation
than your exit?
  

-- Ian


Leave it for now, I am sure jhb or thyco are fine with it,
this is just a heads up FYI for future commits.

Bhyve code has been and still is under a fairly tight
MAINTAINER status.


Modified:
   head/usr.sbin/bhyve/pci_emul.c

Modified: head/usr.sbin/bhyve/pci_emul.c
===
===
--- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019(r3
45170)
+++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019(r3
45171)
@@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
int type)
 bzero(, sizeof(pciecap));

 pciecap.capid = PCIY_EXPRESS;
-   pciecap.pcie_capabilities = PCIECAP_VERSION |
PCIEM_TYPE_ROOT_PORT;
+   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
+   /* Devices starting with version 1.1 must set the RBER bit */
+   if (PCIECAP_VERSION >= 1)
+   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
 pciecap.link_capabilities = 0x411;  /* gen1, x1 */
 pciecap.link_status = 0x11; /* gen1, x1 */









--

--
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)

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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Rodney W. Grimes 
> Em sex, 15 de mar de 2019 ?s 22:12, Ian Lepore  escreveu:
> 
> > On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:
> > > > Author: chuck
> > > > Date: Fri Mar 15 02:11:28 2019
> > > > New Revision: 345171
> > > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > > >
> > > > Log:
> > > >   Fix bhyve PCIe capability emulation
> > > >
> > > >   PCIe devices starting with version 1.1 must set the Role-Based
> > > > Error
> > > >   Reporting bit.
> > > >
> > > >   And while we're in the neighborhood, generalize the code
> > > > assigning the
> > > >   device type.
> > > >
> > > >   Reviewed by:  imp, araujo, rgrimes
> > > >   Approved by:  imp (mentor)
> > > >   MFC after:1 week
> > > >   Differential Revision: https://reviews.freebsd.org/D19580
> > >
> > > This code requires maintainer approval before a commit,
> > > though this was well reviewed that doesnt exclude it
> > > from the MAINTAINERS entry.
> > >
> >
> > Where exactly does it say that in MAINTAINERS?  As another victim of
> > this sort of drive-by lynching after making a trivial bhyve change I
> > pretty seriously object to a vague and meaningless entry in MAINTAINERS
> > being used to pounce on anyone who dares to touch the precious bhyve
> > code.
> >
> 
> There is a new entry on MAINTAINERS:
> https://svnweb.freebsd.org/base?view=revision=344631
> 
> 
> >
> > There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
> > There is an entry for vmm(4), which to me does not say anything about
> > bhyve, yet somehow everybody is supposed to know what it means and
> > what-all territory it covers?
> >
> > IMO, this sort of hyper-proprietary pouncing on everyone who dares
> > change a single line of code is not productive.  It is HIGHLY de-
> > motivating.  Large sweeping design changes are one thing, but pouncing
> > on every tiny minor commit is just not helpful.
> >
> 
> +1
> 
> I got so frustrated with it recently that I have decided to don't
> contribute with bhyve anymore, perhaps even with FreeBSD.
> I still have some people under mentorship that I intend to finish and then
> probably I will phase out.

Your failure to get reviews, and infact even abandon one that had
negative advice as to the validity of your suggested change and
committing it anyway is more likely the cause here.

You also committed code with no review at all that had to be reverted
after the bugs it caused were found by an external down stream consumers
of the bhyve code.

You had code reverted by core due to a external attribution request,
which had you been attending the bi monthly bhyve calls you would of
known was an issue.

I would suggest these are the reasons your feeling angry, and that
I infact tried to reach out to jhb to discuss some of these earlier
but that reach out was never returned.  I under stand your frustration,
you are just wanting to do with best thing you can for the project
and bhyve, can we try to find a better resolution to this situation
than your exit?
 
> > -- Ian
> >
> > > Leave it for now, I am sure jhb or thyco are fine with it,
> > > this is just a heads up FYI for future commits.
> > >
> > > Bhyve code has been and still is under a fairly tight
> > > MAINTAINER status.
> > >
> > > > Modified:
> > > >   head/usr.sbin/bhyve/pci_emul.c
> > > >
> > > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > > ===
> > > > ===
> > > > --- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019(r3
> > > > 45170)
> > > > +++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019(r3
> > > > 45171)
> > > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
> > > > int type)
> > > > bzero(, sizeof(pciecap));
> > > >
> > > > pciecap.capid = PCIY_EXPRESS;
> > > > -   pciecap.pcie_capabilities = PCIECAP_VERSION |
> > > > PCIEM_TYPE_ROOT_PORT;
> > > > +   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > > +   /* Devices starting with version 1.1 must set the RBER bit */
> > > > +   if (PCIECAP_VERSION >= 1)
> > > > +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > > pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > > pciecap.link_status = 0x11; /* gen1, x1 */
> > > >
> > > >
> > > >
> > >
> > >
> >
> >
> >
> 
> -- 
> 
> -- 
> Marcelo Araujo(__)ara...@freebsd.org
> \\\'',)http://www.FreeBSD.org    \/  \ ^
> Power To Server. .\. /_)

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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Rodney W. Grimes 
> On Thu, Mar 14, 2019 at 8:32 PM Rodney W. Grimes 
> wrote:
> 
> > > Author: chuck
> > > Date: Fri Mar 15 02:11:28 2019
> > > New Revision: 345171
> > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > >
> > > Log:
> > >   Fix bhyve PCIe capability emulation
> > >
> > >   PCIe devices starting with version 1.1 must set the Role-Based Error
> > >   Reporting bit.
> > >
> > >   And while we're in the neighborhood, generalize the code assigning the
> > >   device type.
> > >
> > >   Reviewed by:imp, araujo, rgrimes
> > >   Approved by:imp (mentor)
> > >   MFC after:  1 week
> > >   Differential Revision: https://reviews.freebsd.org/D19580
> >
> > This code requires maintainer approval before a commit,
> > though this was well reviewed that doesnt exclude it
> > from the MAINTAINERS entry.
> >
> > Leave it for now, I am sure jhb or thyco are fine with it,
> > this is just a heads up FYI for future commits.
> >
> > Bhyve code has been and still is under a fairly tight
> > MAINTAINER status.
> >
> 
> There is no such thing as a hard lock in FreeBSD. This sounds like you are
> advocating for that, but that's not the case.
> 
> Stop this stupid nitpicking for single line commits. We don't have that
> culture any more and it's really pissing a lot of people off.
> 
> The MAINTAINERS file even says this:
> 
> Please note that the content of this file is strictly advisory.
> 
> And the entry for bhyve doesn't say things are mandatory, just requested.
> 
> Jumping on people's case like this, for a review you yourself were on and
> approved but made no mention of seeking further review / approval, is
> demotivating and toxic. Please stop.

I explicitly DID add jhb to the review.
I also explicitly did not mark the bhyve# box that is added by
the hearald rules.

I did not jump on him, I informed him of the entry, and told him to leave it.
You how ever have infact jumped on me, repeatedly, if you want to talk about
discouraging tones of behavior I suggest you look at yourself as well.

> Warner
> 
> >   head/usr.sbin/bhyve/pci_emul.c
> > >
> > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > >
> > ==
> > > --- head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:27 2019
> > (r345170)
> > > +++ head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:28 2019
> > (r345171)
> > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int
> > type)
> > >   bzero(, sizeof(pciecap));
> > >
> > >   pciecap.capid = PCIY_EXPRESS;
> > > - pciecap.pcie_capabilities = PCIECAP_VERSION | PCIEM_TYPE_ROOT_PORT;
> > > + pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > + /* Devices starting with version 1.1 must set the RBER bit */
> > > + if (PCIECAP_VERSION >= 1)
> > > + pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > >   pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > >   pciecap.link_status = 0x11; /* gen1, x1 */
> > >
> > >
> > >
> >
> > --
> > Rod Grimes
> > rgri...@freebsd.org
> >
> >

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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Warner Losh 
On Thu, Mar 14, 2019 at 8:32 PM Rodney W. Grimes 
wrote:

> > Author: chuck
> > Date: Fri Mar 15 02:11:28 2019
> > New Revision: 345171
> > URL: https://svnweb.freebsd.org/changeset/base/345171
> >
> > Log:
> >   Fix bhyve PCIe capability emulation
> >
> >   PCIe devices starting with version 1.1 must set the Role-Based Error
> >   Reporting bit.
> >
> >   And while we're in the neighborhood, generalize the code assigning the
> >   device type.
> >
> >   Reviewed by:imp, araujo, rgrimes
> >   Approved by:imp (mentor)
> >   MFC after:  1 week
> >   Differential Revision: https://reviews.freebsd.org/D19580
>
> This code requires maintainer approval before a commit,
> though this was well reviewed that doesnt exclude it
> from the MAINTAINERS entry.
>
> Leave it for now, I am sure jhb or thyco are fine with it,
> this is just a heads up FYI for future commits.
>
> Bhyve code has been and still is under a fairly tight
> MAINTAINER status.
>

There is no such thing as a hard lock in FreeBSD. This sounds like you are
advocating for that, but that's not the case.

Stop this stupid nitpicking for single line commits. We don't have that
culture any more and it's really pissing a lot of people off.

The MAINTAINERS file even says this:

Please note that the content of this file is strictly advisory.

And the entry for bhyve doesn't say things are mandatory, just requested.

Jumping on people's case like this, for a review you yourself were on and
approved but made no mention of seeking further review / approval, is
demotivating and toxic. Please stop.

Warner

>   head/usr.sbin/bhyve/pci_emul.c
> >
> > Modified: head/usr.sbin/bhyve/pci_emul.c
> >
> ==
> > --- head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:27 2019
> (r345170)
> > +++ head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:28 2019
> (r345171)
> > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int
> type)
> >   bzero(, sizeof(pciecap));
> >
> >   pciecap.capid = PCIY_EXPRESS;
> > - pciecap.pcie_capabilities = PCIECAP_VERSION | PCIEM_TYPE_ROOT_PORT;
> > + pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > + /* Devices starting with version 1.1 must set the RBER bit */
> > + if (PCIECAP_VERSION >= 1)
> > + pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> >   pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> >   pciecap.link_status = 0x11; /* gen1, x1 */
> >
> >
> >
>
> --
> Rod Grimes
> rgri...@freebsd.org
>
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Marcelo Araujo 
Em sex, 15 de mar de 2019 às 22:12, Ian Lepore  escreveu:

> On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:
> > > Author: chuck
> > > Date: Fri Mar 15 02:11:28 2019
> > > New Revision: 345171
> > > URL: https://svnweb.freebsd.org/changeset/base/345171
> > >
> > > Log:
> > >   Fix bhyve PCIe capability emulation
> > >
> > >   PCIe devices starting with version 1.1 must set the Role-Based
> > > Error
> > >   Reporting bit.
> > >
> > >   And while we're in the neighborhood, generalize the code
> > > assigning the
> > >   device type.
> > >
> > >   Reviewed by:  imp, araujo, rgrimes
> > >   Approved by:  imp (mentor)
> > >   MFC after:1 week
> > >   Differential Revision: https://reviews.freebsd.org/D19580
> >
> > This code requires maintainer approval before a commit,
> > though this was well reviewed that doesnt exclude it
> > from the MAINTAINERS entry.
> >
>
> Where exactly does it say that in MAINTAINERS?  As another victim of
> this sort of drive-by lynching after making a trivial bhyve change I
> pretty seriously object to a vague and meaningless entry in MAINTAINERS
> being used to pounce on anyone who dares to touch the precious bhyve
> code.
>

There is a new entry on MAINTAINERS:
https://svnweb.freebsd.org/base?view=revision=344631


>
> There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
> There is an entry for vmm(4), which to me does not say anything about
> bhyve, yet somehow everybody is supposed to know what it means and
> what-all territory it covers?
>
> IMO, this sort of hyper-proprietary pouncing on everyone who dares
> change a single line of code is not productive.  It is HIGHLY de-
> motivating.  Large sweeping design changes are one thing, but pouncing
> on every tiny minor commit is just not helpful.
>

+1

I got so frustrated with it recently that I have decided to don't
contribute with bhyve anymore, perhaps even with FreeBSD.
I still have some people under mentorship that I intend to finish and then
probably I will phase out.


>
> -- Ian
>
> > Leave it for now, I am sure jhb or thyco are fine with it,
> > this is just a heads up FYI for future commits.
> >
> > Bhyve code has been and still is under a fairly tight
> > MAINTAINER status.
> >
> > > Modified:
> > >   head/usr.sbin/bhyve/pci_emul.c
> > >
> > > Modified: head/usr.sbin/bhyve/pci_emul.c
> > > ===
> > > ===
> > > --- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019(r3
> > > 45170)
> > > +++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019(r3
> > > 45171)
> > > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
> > > int type)
> > > bzero(, sizeof(pciecap));
> > >
> > > pciecap.capid = PCIY_EXPRESS;
> > > -   pciecap.pcie_capabilities = PCIECAP_VERSION |
> > > PCIEM_TYPE_ROOT_PORT;
> > > +   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > > +   /* Devices starting with version 1.1 must set the RBER bit */
> > > +   if (PCIECAP_VERSION >= 1)
> > > +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > > pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > > pciecap.link_status = 0x11; /* gen1, x1 */
> > >
> > >
> > >
> >
> >
>
>
>

-- 

-- 
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-15 Thread Ian Lepore 
On Thu, 2019-03-14 at 19:31 -0700, Rodney W. Grimes wrote:
> > Author: chuck
> > Date: Fri Mar 15 02:11:28 2019
> > New Revision: 345171
> > URL: https://svnweb.freebsd.org/changeset/base/345171
> > 
> > Log:
> >   Fix bhyve PCIe capability emulation
> >   
> >   PCIe devices starting with version 1.1 must set the Role-Based
> > Error
> >   Reporting bit.
> >   
> >   And while we're in the neighborhood, generalize the code
> > assigning the
> >   device type.
> >   
> >   Reviewed by:  imp, araujo, rgrimes
> >   Approved by:  imp (mentor)
> >   MFC after:1 week
> >   Differential Revision: https://reviews.freebsd.org/D19580
> 
> This code requires maintainer approval before a commit,
> though this was well reviewed that doesnt exclude it
> from the MAINTAINERS entry.
> 

Where exactly does it say that in MAINTAINERS?  As another victim of
this sort of drive-by lynching after making a trivial bhyve change I
pretty seriously object to a vague and meaningless entry in MAINTAINERS
being used to pounce on anyone who dares to touch the precious bhyve
code.

There is no mention of bhyve in MAINTAINERS, for usr.sbin or elsewhere.
There is an entry for vmm(4), which to me does not say anything about
bhyve, yet somehow everybody is supposed to know what it means and
what-all territory it covers?

IMO, this sort of hyper-proprietary pouncing on everyone who dares
change a single line of code is not productive.  It is HIGHLY de-
motivating.  Large sweeping design changes are one thing, but pouncing
on every tiny minor commit is just not helpful.

-- Ian

> Leave it for now, I am sure jhb or thyco are fine with it,
> this is just a heads up FYI for future commits.
> 
> Bhyve code has been and still is under a fairly tight
> MAINTAINER status.
> 
> > Modified:
> >   head/usr.sbin/bhyve/pci_emul.c
> > 
> > Modified: head/usr.sbin/bhyve/pci_emul.c
> > ===
> > ===
> > --- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019(r3
> > 45170)
> > +++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019(r3
> > 45171)
> > @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi,
> > int type)
> > bzero(, sizeof(pciecap));
> >  
> > pciecap.capid = PCIY_EXPRESS;
> > -   pciecap.pcie_capabilities = PCIECAP_VERSION |
> > PCIEM_TYPE_ROOT_PORT;
> > +   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> > +   /* Devices starting with version 1.1 must set the RBER bit */
> > +   if (PCIECAP_VERSION >= 1)
> > +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> > pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> > pciecap.link_status = 0x11; /* gen1, x1 */
> >  
> > 
> > 
> 
> 

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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-14 Thread Conrad Meyer 
On Thu, Mar 14, 2019 at 8:06 PM Andrew Thompson  wrote:
>
> On Fri, 15 Mar 2019 at 15:11, Chuck Tuffli  wrote:
>> bzero(, sizeof(pciecap));
...
>> +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
>
> If the message you say 'set the bit' but you are overwriting the whole 
> variable, is this intended?

Looks like it was zero before.  So yeah, it sets the bit.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-14 Thread Andrew Thompson 
On Fri, 15 Mar 2019 at 15:11, Chuck Tuffli  wrote:

> Author: chuck
> Date: Fri Mar 15 02:11:28 2019
> New Revision: 345171
> URL: https://svnweb.freebsd.org/changeset/base/345171
>
> Log:
>   Fix bhyve PCIe capability emulation
>
>   PCIe devices starting with version 1.1 must set the Role-Based Error
>   Reporting bit.
>
>   And while we're in the neighborhood, generalize the code assigning the
>   device type.
>
>   Reviewed by:  imp, araujo, rgrimes
>   Approved by:  imp (mentor)
>   MFC after:1 week
>   Differential Revision: https://reviews.freebsd.org/D19580
>
> Modified:
>   head/usr.sbin/bhyve/pci_emul.c
>
> Modified: head/usr.sbin/bhyve/pci_emul.c
>
> ==
> --- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019
> (r345170)
> +++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019
> (r345171)
> @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int type)
> bzero(, sizeof(pciecap));
>
> pciecap.capid = PCIY_EXPRESS;
> -   pciecap.pcie_capabilities = PCIECAP_VERSION | PCIEM_TYPE_ROOT_PORT;
> +   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> +   /* Devices starting with version 1.1 must set the RBER bit */
> +   if (PCIECAP_VERSION >= 1)
> +   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
> pciecap.link_capabilities = 0x411;  /* gen1, x1 */
> pciecap.link_status = 0x11; /* gen1, x1 */
>

If the message you say 'set the bit' but you are overwriting the whole
variable, is this intended?


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

Re: svn commit: r345171 - head/usr.sbin/bhyve

2019-03-14 Thread Rodney W. Grimes 
> Author: chuck
> Date: Fri Mar 15 02:11:28 2019
> New Revision: 345171
> URL: https://svnweb.freebsd.org/changeset/base/345171
> 
> Log:
>   Fix bhyve PCIe capability emulation
>   
>   PCIe devices starting with version 1.1 must set the Role-Based Error
>   Reporting bit.
>   
>   And while we're in the neighborhood, generalize the code assigning the
>   device type.
>   
>   Reviewed by:imp, araujo, rgrimes
>   Approved by:imp (mentor)
>   MFC after:  1 week
>   Differential Revision: https://reviews.freebsd.org/D19580

This code requires maintainer approval before a commit,
though this was well reviewed that doesnt exclude it
from the MAINTAINERS entry.

Leave it for now, I am sure jhb or thyco are fine with it,
this is just a heads up FYI for future commits.

Bhyve code has been and still is under a fairly tight
MAINTAINER status.

> Modified:
>   head/usr.sbin/bhyve/pci_emul.c
> 
> Modified: head/usr.sbin/bhyve/pci_emul.c
> ==
> --- head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:27 2019
> (r345170)
> +++ head/usr.sbin/bhyve/pci_emul.cFri Mar 15 02:11:28 2019
> (r345171)
> @@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int type)
>   bzero(, sizeof(pciecap));
>  
>   pciecap.capid = PCIY_EXPRESS;
> - pciecap.pcie_capabilities = PCIECAP_VERSION | PCIEM_TYPE_ROOT_PORT;
> + pciecap.pcie_capabilities = PCIECAP_VERSION | type;
> + /* Devices starting with version 1.1 must set the RBER bit */
> + if (PCIECAP_VERSION >= 1)
> + pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
>   pciecap.link_capabilities = 0x411;  /* gen1, x1 */
>   pciecap.link_status = 0x11; /* gen1, x1 */
>  
> 
> 

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

svn commit: r345171 - head/usr.sbin/bhyve

2019-03-14 Thread Chuck Tuffli 
Author: chuck
Date: Fri Mar 15 02:11:28 2019
New Revision: 345171
URL: https://svnweb.freebsd.org/changeset/base/345171

Log:
  Fix bhyve PCIe capability emulation
  
  PCIe devices starting with version 1.1 must set the Role-Based Error
  Reporting bit.
  
  And while we're in the neighborhood, generalize the code assigning the
  device type.
  
  Reviewed by:  imp, araujo, rgrimes
  Approved by:  imp (mentor)
  MFC after:1 week
  Differential Revision: https://reviews.freebsd.org/D19580

Modified:
  head/usr.sbin/bhyve/pci_emul.c

Modified: head/usr.sbin/bhyve/pci_emul.c
==
--- head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:27 2019
(r345170)
+++ head/usr.sbin/bhyve/pci_emul.c  Fri Mar 15 02:11:28 2019
(r345171)
@@ -953,7 +953,10 @@ pci_emul_add_pciecap(struct pci_devinst *pi, int type)
bzero(, sizeof(pciecap));
 
pciecap.capid = PCIY_EXPRESS;
-   pciecap.pcie_capabilities = PCIECAP_VERSION | PCIEM_TYPE_ROOT_PORT;
+   pciecap.pcie_capabilities = PCIECAP_VERSION | type;
+   /* Devices starting with version 1.1 must set the RBER bit */
+   if (PCIECAP_VERSION >= 1)
+   pciecap.dev_capabilities = PCIEM_CAP_ROLE_ERR_RPT;
pciecap.link_capabilities = 0x411;  /* gen1, x1 */
pciecap.link_status = 0x11; /* gen1, x1 */
 
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"